fix(identity): resolve a key's private half by matching material, not by deriving it from purpose (#889) - #946
Merged
Conversation
Issue #889: a v0.9.3 identity that was already partially loaded before the upgrade keeps its remaining keys stranded in `data.db`, because the migration skips a present identity wholesale. That rule is correct — three earlier reconcile heuristics each failed by inferring user intent from record shape. `compute_recovery_plan` only lists what the legacy record holds and the modern one does not, keyed on the `(target, key_id)` map slot and on `None` associations; it never decides whether an item should come back. That decision travels back in as an item-level allowlist, so the absence-versus-removal ambiguity lands with the only party holding the provenance. `apply_recovery_plan` is additive by construction, not by discipline: it starts from the fresh modern record, inserts with `or_insert` so the modern copy wins every collision, recomputes candidacy rather than trusting the allowlist, and has no write path at all for alias, status, the dpp identity, DPNS names, or the wallet link. A voting key drags its voter-identity link along, since one without the other cannot vote. M1-M10 from the design doc's test matrix. M2/M5/M6/M9/M10 were confirmed RED against a naive legacy-wins merge before this implementation landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ecoder Issue #889: the recovery flow needs one identity out of the preserved legacy `data.db`, not the whole table. A second hand-written reader would be free to drift from the importer's idea of which rows are readable — and then recovery could merge from a record the migration never considered importable, or miss one it did. So the per-row decode moves into `decode_identity_row` and both readers call it, along with one shared `SELECT` that binds the same pair of accepted network spellings. `read_identity_row` adds only the `id` predicate on top. `LegacyIdentityLookup` keeps "nothing to recover" and "the row will not decode" apart. An absent table, an absent row, an observed-identity cache row and a NULL blob are all `Absent` — the ordinary answer. Only a genuinely corrupt row is `Unreadable`, so no caller can mistake corruption for an empty record and merge against it. `open_legacy_read_only` is hoisted next to `database/mod.rs`'s existing one, so `SQLITE_OPEN_READ_ONLY` is a property of the one function that opens the file rather than of each caller remembering the flag. The importer's 12 existing row-decode tests are the regression net for the extraction; 8 new tests cover the lookup, including one asserting both readers decode the same row identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… data Issue #889: an identity that was only partially loaded before the v1.0 upgrade keeps its remaining keys in `data.db`, reachable by nothing — the load form rejects the duplicate ProTxHash, and the per-key screen wants a WIF the user may no longer hold. This is the way in, and it is entirely user-driven. `CheckLegacyRecovery` lists; `RecoverLegacyIdentityData` writes. Detection is read-only, offline and side-effect-free, so a screen can dispatch it on arrival and hide the affordance when the plan comes back empty. Candidacy is recomputed inside the executing task rather than trusted from the caller: only `recomputed-candidates ∩ approved` is merged, so an approval that went stale since the preview is reported rather than acted on, and an item the user never approved is never restored even when it is missing. An empty allowlist is refused, not widened into "restore everything". The protection-downgrade trip that sank an earlier reconcile attempt is unreachable here by branch condition, not by care: the flow branches on the same predicate the at-rest guard evaluates. On a protected identity the password is verified up front through the shipped prompt, the resident-plaintext preflight runs before any vault write, and every merged key is sealed under that password before the record is persisted — so the encoder never sees plaintext on a protected identity. On a keyless identity there is no protected key for the guard to protect. Cancel, wrong password and headless all fail closed with the stored record byte-identical. Exactly one record write, so the merge either lands whole or changes nothing. No new persisted state: eligibility recomputes from the two existing stores, so a re-run is a no-op and `data.db` — opened read-only — stays a repeatable recovery source. `reject_resident_identity_plaintext` widens to `pub(super)`; it now guards both seal boundaries. Two test-only readers expose the at-rest blob and the wallet link, which the hydrated record cannot show. B1-B12 from the design doc's test matrix, on the offline wired `AppContext` harness, plus the excluded-key and empty-allowlist cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
…node page Issue #889: the restore path exists but nothing reaches it. The node detail page is the canonical surface — its operator is the one holding a node that cannot vote because its voting key stayed behind in the previous version's saved data. The offer is passive and contextual, never a launch-time nag. On opening a node the view dispatches the read-only detection task once, and only on an install that actually has a previous-version database; the section renders in the Keys area only when detection found something, and retires itself once a restore lands, because eligibility recomputes from the two stores every time. For a node with no voting key that becomes the primary remedy: the missing-voter message points at restoring, which asks nothing of an operator who no longer has the key, and the existing WIF prompt keeps its place as the fallback for one who does. `LegacyRecoveryState` (`ui/state/`) owns the fetch state — dispatch-once, in-flight, offered, retryable — and `LegacyRecoverySection` (`ui/components/`) renders it, per the placement rule's render/no-render split. A failed restore returns to its offer so a mistyped identity password can be corrected and retried; a failed detection does not re-ask. A preview result is routed into the open detail view rather than triggering the list screen's reload-and-reopen, which would rebuild the view, re-dispatch its check and never settle. A completed restore does reload, since the store changed, and the rebuilt view's fresh check finds nothing left. `RecoveryItemDescriptor::is_voting_key` factors out the rule `label` already applied, so "is this the voting key" has one definition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
Issue #889: the node page covers the masternode case, but a `User` identity that was partially loaded before the upgrade has no node page. The Key Info screen is the issue's own suggested location and the one surface every identity type reaches — from the Identities list for a user identity, and through Manage keys for a node. The offer is identity-scoped rather than key-scoped, so it renders whatever the opened key's own state is: what it lists are precisely the keys the identity does *not* hold, which no per-key view could show. Both surfaces share `LegacyRecoveryState` and `LegacyRecoverySection`, so detection, the allowlist and the copy have one definition each. Dispatch follows this screen's established queue-then-drain pattern, alongside the protect/unprotect ones. The check and a restore can never contend for the frame's single `AppAction`: the check goes out once on arrival, and a restore only after a press. A completed restore re-probes the protection status, since the restored keys have just landed in the vault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
…ration gap Issue #889: the migration design's §7 named this limitation and deferred it to "a dedicated, provenance-aware flow". That flow now exists, so the limitation gets its resolution rather than staying an open pointer to an issue — with the one clarification a reader of that section needs: the importer itself did not change, skip-if-present still stands, and nothing about recovery runs at migration or launch time. The design record moves into `docs/ai-design/` under this repo's dated-directory convention, so the cross-reference resolves inside the repo. U1 covers the widget from the outside: an empty plan renders nothing, items are named by role with the voter link folded into its voting key, pressing Restore approves exactly the previewed set, an in-flight restore has no button to press twice, unrestorable keys are listed with no restore offered, and an install with no previous-version data never even asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The plan admitted any legacy key the modern record happened not to hold,
keyed on the map slot alone. Nothing checked that the saved private half
derived the public half it was stored with, that the public half was still
a live key of the identity, or that a wallet-derivation reference named a
wallet this install holds — the exact checks the manual "type the WIF"
path this flow replaces already enforces.
That matters because `masternode_key_presence` reports a role as held from
the record alone. Restoring a key the node rotated away from flipped the
role to "present", retiring both the recovery offer and the missing-voter
remedy, and the operator found out from a rejected transaction. The voter
link alone did the same thing with no key behind it at all.
Failing candidates now go to `excluded` with their own reason and are
never restorable. `AppliedRecovery` also reports that `excluded` list, so
an approval naming something that was never restorable is no longer also
reported as a stale approval ("already back in place" — the opposite
answer), and the caller stops recomputing the same plan to read one field
off it. Whatever plaintext the legacy record still holds after a merge is
zeroized instead of being dropped intact.
The item labels and exclusion copy move out of `model/` into the UI, where
they now delegate to the shipped `role_label_and_tip` vocabulary rather
than a second mapping that disagreed with it on the same screen ("Payout
key" next to "Payout address key"), and two rows in one role are told
apart by key id instead of rendering identically.
Test fixtures move onto genuinely derived key pairs with the chain's
public keys published on the record — a correspondence check that passes
against placeholder key material verifies nothing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
`recover_legacy_identity_data` took `migration_run` for the whole async span, prompt included. The delete path it copied that pattern from is fully synchronous and holds the mutex for microseconds; this held it for as long as a modal sat in front of the user. Two independent reviewers reproduced the fallout: removing an *unrelated* identity failed with "storage is still being updated" when no migration was running, and `finish_unwire`'s awaiting acquire could park indefinitely. The flow is now a preflight and one fully synchronous critical section. The preflight reads, dry-runs the merge to see whether anything would be restored at all, decides whether a password is needed, and prompts — holding nothing. The write section then re-acquires the guard, re-checks the migration state, re-reads the record, merges again and writes, all without an await in between. Re-reading is not just tidiness. The prompt window let any other writer land in between — refresh, DPNS registration, transfer, none of which take the load claim — and the write of a pre-prompt snapshot silently reverted it. It also makes the no-resurrection guarantee explicit: a delete during the prompt now surfaces as `IdentityNotFoundLocally` instead of being undone by the upsert, which until now held only as a side effect of how long the lock was kept. The at-rest protection predicate is evaluated over the merged record, the one the encoder's downgrade guard actually sees, rather than over the pre-merge record. If the identity gained password protection during the prompt the verified password no longer covers what is about to be written, so the restore stops with a typed `LegacyRecoveryIdentityChanged` rather than sealing under a password for a different state. Also: the nothing-to-recover success no longer records a `Failed` load, the preview zeroizes the stranded plaintext it decodes on every screen open instead of releasing it intact, and the "nothing restored" banner is worded to be true of both outcomes that produce it. Both concurrency tests were confirmed RED against the old lock scope, reproducing the reviewers' exact failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
Three ways the two shipped surfaces disagreed with the store after a restore, and the state machine behind them had no tests at all. The Key Info screen keeps the identity clone it opened with, and its own add-key and remove-key paths persist that whole clone. This feature added a second writer to the same record reachable from the same screen: restore keys, then add or remove any key, and the pre-restore copy was written back — the restored keys gone, right after a banner said they were back. The completion now re-reads the record and the key on screen from it. The node page never hears about a restore run from the Key Info screen it pushed: the pushed screen is on top, so it takes the result. Coming back left the page offering keys already restored and warning about a voting key it now held. Arrival re-reads the node and re-arms its check; vote selections and any open prompt survive, which a blanket rebuild would have discarded. The `recovery_completed()` call in the result handler goes with it — the reload that follows re-opens the view and re-arms the check anyway, so it was mutating a view discarded three statements later. Writing the state machine's tests turned up a third one: `completed()` re-armed unconditionally, so calling it on an install with no previous-version database armed a check that must never run. Harmless until arrival started calling it; now `Unavailable` stays unavailable. The Restore button loses its ellipsis: on a keyless identity — the common case this feature exists for — it commits immediately rather than opening anything. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The committed design record was landed marked "implemented" with exactly one edit — the status line — so the repo's permanent account of this feature described a dispatch model, a progress mechanism, a module set and a gating rule that were all deliberately changed, plus the pre-review merge rules that the fix round replaced. A §10 now records all nine departures with the reasoning that produced them, rather than rewriting §1–§9 into a story nobody actually followed; the status line says which of the two is authoritative. §7's claim that `finish_unwire.rs` is untouched is reconciled with §4.2's hoist in the same document: the importer's behaviour is unchanged, the file is not. `reject_resident_identity_plaintext`'s header claimed to guard "every boundary that seals an identity's keys". It does not — the merge-load path seals without it. The header now says which boundaries it covers, and that path carries a TODO describing the gap. Fixing it is a separate issue: it is pre-existing code, unrelated to this feature. Also: user story and CHANGELOG pick up the correspondence check and the non-blocking prompt, the components catalog names the copy this feature's component now owns, and three private rustdoc blocks come back inside the internal-commentary budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The correspondence check that keeps a rotated-away key out of the offer resolved a voter/operator-target key's identity as "the modern link, or the legacy one if there is no modern link". In the case the check exists for -- a masternode whose record carries no voter link -- that fallback read the identity snapshot out of the same unauthenticated legacy blob as the key under test, so any self-consistent pair passed. A voting key the chain retired months before the upgrade still reads as live in that snapshot, and restoring it flips the node's voting role to "present", retiring the manual remedy that would actually fix the node. reference_identity now reads the modern record only. A key on a voter or operator identity the modern record does not link to has no admissible witness and is excluded as LinkedIdentityUnverified, carrying the same "load this identity again and enter the key" remedy as its neighbours. Fetching the linked identity from Platform was rejected twice over. It would put the network in the middle of an offline, read-only preview that every screen arrival dispatches, and model/ is pure by contract. More decisively, it would not establish the property: a voter identity's id is derived from its voting key (Identifier::create_voter_identifier), so rotating the key on-chain creates a *different* voter identity rather than retiring a key on the existing one. Fetching the legacy-named voter identity would confirm the stale key against its own orphaned identity. Behaviour change worth noticing on its own: a masternode loaded from its ProTxHash alone no longer gets its voter-identity-target voting key back in one click. The key is listed as unverifiable, the voter link goes with it, the node keeps reporting the role as missing, and render_missing_voter keeps offering manual entry -- the honest outcome, since nothing available offline can tell that key from one the chain replaced. A record that does carry the voter link (a re-load keeps it) restores the key as before. Grouping follows the same rule: voter_association_is_grouped now keys on the voting role, the predicate that already decides whether the link is worth offering, so the two cannot disagree about which key the link belongs to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
… existing FetchState::Unavailable was documented as the state a fresh install never leaves, but no shipped install could reach it. AppState::boot_inputs opens an existing data.db read-only *or* creates one and runs the fresh-install schema ladder, so db_file_path().exists() is true for every user, and every identity or node screen dispatched a detection task against a table the ladder had just created empty. The kittest that claimed otherwise passed only because the `testing` feature substitutes an in-memory database whose path is None -- a test of the substitution, not of the gate. AppContext::has_legacy_identities now asks whether the legacy `identity` table holds a local row for this network: one SELECT EXISTS over the same filter read_identities uses (so "there is something here" and "here it is" cannot drift), false when the table does not exist at all, answered once per context and cached. A probe that errors arms the offer instead of retiring it -- the detection task reports its own typed error, while a silent false would withdraw a recovery the user's data still supports. The kittest is replaced by lib tests over a real file-backed data.db, which assert the premise the old gate missed (the file is there and the gate still says no) and cover the network scoping a file check could never have. Confirmed red against the old gate before the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
…estore Two ways a restore's result reached the wrong screen state. KeyInfoScreen had an empty refresh(), which refresh_on_arrival defaults to, so returning to a screen that missed its result never re-read the record. That screen persists the whole identity clone it opened with on every key add or remove, so the clone -- predating the restore -- was written back over the restored keys on the next edit, silently and with a successful save to show for it. It now re-reads on arrival, re-probes the protection line and re-arms its check, exactly as the masternode detail view already does. The LegacyRecoveryCompleted arm discarded identity_id, unlike the LegacyRecoveryCandidates arm directly above it, which routes through LegacyRecoveryState::offered's identity check. Results reach whichever screen is visible when they arrive, not the screen that dispatched them, so a restore dispatched from identity A's Key Info screen could land on identity B's: B showed "your keys have been restored to this identity" for a restore that was never B's, and B's own in-flight recovery state was reset under it. The attribution rule now lives once, in LegacyRecoveryState as completed_for -- the twin of offered -- so both call sites share it rather than each remembering to check. The masternodes list applies the same rule through shows_node: a completion for an identity no card and no open detail names is dropped, banner, reload and detail rebuild included. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
TaskError::LegacyRecoveryIdentityChanged is listed in persist_legacy_recovery's rustdoc and spelled out in design.md 10.6, but was the only documented error branch of that function with no test -- every sibling (IdentityLoadInProgress, WalletStorageNotReady, IdentityNotFoundLocally, LegacyIdentityUnreadable, the prompt's cancel and unavailable errors, IdentityKeyProtectionIncomplete, the deleted identity, the undecodable row) has one. B13 drives the 10.6 scenario: the dry run on a keyless identity decides no password is needed, a ProtectIdentityKeys lands before the write section re-reads the record, and the write refuses with the typed error and a byte-identical stored blob. It calls persist_legacy_recovery directly with the None the dry run produced, as B5 calls verify_recovery_password directly: a Tier-1 restore never prompts, so there is no await point between the two where a concurrent task could be made to land deterministically. The dry run is exercised first so the None under test is the one production carries. Verified the test fails when the fail-closed branch is removed, rather than merely restating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The voter-target fix landed one half of the rule: a key on a voter or operator identity the modern record does not link to has no witness outside the legacy file, so it is excluded. The operator *link* itself was offered with no gate at all, which left the same hole open one step further out. Restoring it writes the legacy file's own claim into the modern record. The next preview then reads that claim back as the outside witness for the same file's operator keys, and every one of them becomes a candidate — self-vouching in two moves instead of one. Nothing in production ever writes `associated_operator_identity` fresh, so an offered operator link was always in that unverifiable state. An operator link the modern record does not carry is now excluded as `LinkedIdentityUnverified`, like the keys held on that identity. Also renames M8, whose name and opening sentence claimed it proved the whole legacy key set is offered while its own assertions exclude two of five items.
The node page carried a second missing-voter message offering restore as the primary remedy, chosen whenever the recovery plan held a voting-role candidate. Since the voter-key check began reading the modern record only, that candidate exists only when the record already carries the voter link — and a record carrying the voter link is written in the same branch that writes the voting key, so it is never in the missing-voter state to begin with. The two conditions are mutually exclusive: no user, and no test, has ever seen that message or its companion tooltip. Gone, along with the predicate behind them. The remaining message is the honest one and still matches what the screen offers: the in-place "Add voting key" prompt right underneath it, which needs no re-load. Restoring a voting key held on a separate voter identity needs an on-chain check this flow deliberately does not make; that is issue #942, not something this branch pretends to do.
… nothing A plan holding only unrestorable items — now the ordinary shape for a masternode loaded from its ProTxHash — opened with "Some keys ... haven't been brought across", followed by no list, no button, and then a second heading for the items it cannot bring back. A lead-in that promises an action the section never offers reads as a broken screen. That case gets its own lead-in, which introduces the list itself, so the second heading appears only when there really are two lists to keep apart. The reason an item cannot be restored also stops being hover-only. It is the single sentence that tells the user what to do instead, and a tooltip reaches neither touch nor keyboard; it is now an inline line under each item. The reason for an unverifiable linked identity is reworded to read correctly for the operator link as well as for a key held on one.
The user story and the changelog both said restoring is offered as the first remedy for a node with no voting key, with typing the key in as the fallback. For the flagship case — a masternode loaded from its ProTxHash alone — that is never true: the voting key sits on a separate voting identity that only the previous version's data names, so it is listed as one that cannot be brought back and entering it by hand is the only remedy. Naming owner, voting and payout together in the same breath carried the same implication and is dropped. Both now describe what happens, and point at issue #942 for the on-chain check that would let the voting-key case be closed safely later. The design record gains the two shipped deviations behind this: §2.1's "restore first, WIF as fallback" branch removed as unreachable, and the operator link excluded as unverifiable for the same reason its keys are.
The identity hub replaced the legacy Identities screen's per-key popup — the only route into KeyInfoScreen that did not depend on the identity already holding a usable key — with Settings > Advanced > "Manage keys". That destination was a read-only table with no route onward, and RootScreenIdentities is deliberately out of the nav, so the popup it still hosts is unreachable. Every remaining route (transfer, withdraw, the token screens) is gated on holding a key of the kind that action needs, which is false for precisely the identities issue #889's restore offer exists to help: a user identity with stranded keys could reach its own keys only in Developer view, through a send-money screen. KeysScreen now carries the QualifiedIdentity, so it can say which keys this device holds and open KeyInfoScreen with that key's private material. One row per key, named in the shared role vocabulary, held state stated in words rather than by colour alone, ungated. The restore offer renders on the list itself, above the rows: it is scoped to the identity, and making a user pick an arbitrary key to discover an identity-level offer repeats the defect one level up. It stays on KeyInfoScreen too, which self-extinguishes and is where the masternode path lands. The screen also moves inside the app chrome (top panel, left panel, island_central_panel). That is not cosmetic: island_central_panel is the only caller of MessageBanner::show_global, so on a raw CentralPanel the restore reports nothing at all — a failed restore would be indistinguishable from a successful one. key_role_label, manage_keys_labels and the key enumeration move from detail_screen into ui::masternodes beside the vocabulary they belong to, so the two "Manage keys" surfaces cannot drift apart; the masternode kittest covering that list is the regression guard for the move. The transfer screen's no-key branch gains a "Manage keys" signpost. Its available_transfer_keys() gate is untouched — it still correctly says the identity cannot send; the branch simply stops being a dead end.
…s list The offer self-extinguishes on the next check whether the restore landed or failed, so its disappearance says nothing about the outcome. Only the banner distinguishes the two, and the banner only reaches the user because the screen is built on island_central_panel — the sole caller of MessageBanner::show_global. Cover both outcomes explicitly, and pin the render channel itself: reverting the screen to a bare CentralPanel fails the new guard, which is what makes it worth having. Also pin AC-6 across the two surfaces that name keys. The restore offer and the keys list render from different modules over different types, and nothing but their shared derivation from role_label_and_tip keeps them saying the same words; a private re-implementation on either side would read plausibly and drift in silence. Key Info remains outside that assertion: it does not retain the PrivateKeyTarget its key was opened with, so it cannot tell a voter-identity key from a main-identity one, and naming it in role words today would risk labelling a voting key as an authentication key. Threading the target through its nine call sites is the fix, and it is wider than this change.
…running app The component tests prove two things separately: the screen sets a banner on a finished restore, and it renders whatever banner its context carries. Neither closes the seam between them — that the context the screen writes its outcome into is the context it renders from. A restore reporting into one context while the screen renders another would pass both and still tell the user nothing. This drives the real app to the keys list, delivers a completed restore through visible_screen_mut the way AppState does, and asserts the success text appears on the screen; then banners a task error on the app context, as AppState does for a failed restore, and asserts that appears too. The offer retires itself either way, so the banner is the only thing that distinguishes them. Reverting the screen to a bare CentralPanel fails this test, which is what makes it a guard rather than a decoration.
…s filed SEC-001. The keys list resolves each key's PrivateKeyTarget from the identity it walked, then discarded it; KeyInfoScreen re-derived one from the key's purpose. impl From<Purpose> for PrivateKeyTarget sends every voting key to the voter identity, so for a Purpose::VOTING key filed on the main identity — a supported shape, which masternode_key_presence reads as voting readiness on its own — the two disagree. The list reports the key as held, Key Info re-reads the record on arrival, looks in a store the key was never filed under, finds nothing, and reports it missing. On the screen built to answer that question, about a key whose private half is listed one screen earlier. KeyInfoScreen now accepts the resolved target via with_target and uses it for every lookup, falling back to the old derivation when no caller supplied one, so the call sites this change does not touch keep their current behaviour. Only the keys list passes it. The same derivation still governs the pre-existing callers' reads, writes and deletes — including a delete that can land on a different key when voter and main key-id spaces overlap — which is filed separately. Also from QA: - SEC-002: the held/not-held column called get_cloned_private_key_data_and_ wallet_info, copying raw key bytes out of the vault unscrubbed, every frame for every key, to read is_some(). KeyStorage::has answers it without a copy. - QA-005: display_message ended a restore on any MessageType::Error. Results are not screen-affine, so an unrelated task's failure re-armed Restore mid-flight and it could be dispatched twice. Attribution moves to display_task_error, which receives the typed error; it returns false so the user still gets AppState's banner. Residual documented on is_recovery_error: the same variant from another task during a restore still ends it, which needs the task's identity to reach the screen. - QA-003: the completion-banner test asserted only that some banner existed. It now pins attribution instead — another identity's restore reports nothing here — since fresh_app_context drops the harness its context belongs to and the text cannot be read back at that level. The running-app test already pins the text. - QA-006/007: coverage for the transfer-screen signpost and for the Expert-only detail row in both directions. - Empty-state copy said "no keys saved on this device", which in this screen's own per-key vocabulary means held=false. An empty record is a different statement; the test that codified the wrong string is fixed with it. - QA-002: "island_central_panel is the only caller of show_global" is false — contracts_documents_screen calls it directly. Corrected where it was written down as justification. - key_role_label back to pub(crate); ui/masternodes' module header no longer implies its key vocabulary is Expert-Mode-scoped, since an Everyday identity screen depends on it. - Docs: §2.1 and §7 row 1 restored as written, per the document's own proposal-plus-deviations model, with §10.13 carrying the correction. CHANGELOG no longer claims transfer was the only route to a key's page, and no longer promises signing or password protection to an identity that holds no keys yet.
… own words Track A of round 3. Leaving a key now pops with a refresh, and the keys list re-reads both halves of its state on the way back. Only re-reading the record would show a key restored from the pushed Key Info screen as held while still offering to restore it — and pressing Restore then reports there was nothing left to do. Consolidating into refresh() also retires the refresh_on_arrival override, which never ran: the framework dispatches that hook for root screens, and this screen is pushed. KeyInfoScreen gains a with_parent breadcrumb crumb so the trail leads back to the screen the key was opened from. Parameterised, not hardcoded: it has nine parents and naming one of them for all of them would mislabel the other eight, which keep their two-level trail unchanged. Key role words now follow the identity that owns the key. The DIP-3 wording describes masternode registration duties — updating a registration, receiving node rewards — and on a user identity that does not read as jargon, it asserts the user owns a masternode. A user's TRANSFER key is a Transfer key, not a payout address. Both the keys list and the restore offer take the same vocabulary from the same identity: scoping only one would have a user's offer say "Payout address key" six lines above a list saying "Transfer key", which is the two-names-for-one-key defect the shared vocabulary exists to prevent. Empty-state copy no longer says "no keys saved on this device" — these rows are the identity's on-chain public keys, and that phrase means held=false per-row on this same screen. Track B, reads only, after verifying the read/write split against the source rather than the report: :746 (feeds the vault chokepoint, whose consequence is signing with and displaying the wrong key's material) and :887 (reload) are reads and now resolve the target the caller established. The keys list also resolves held-ness against both conventions, since real installs hold material written under each; looking in a second place can only find material that is already there. The writes are deliberately reverted to the purpose-derived target, which the previous commit had changed. QualifiedIdentity::sign and can_sign_with resolve that way, so material stored anywhere else is material that can never sign: for a voting key filed on the main identity, the previous commit would have taken a hand-entered private key and stored it where the signer never looks. Removal keeps the derived target for the same reason, which leaves wrong-key removal as a documented residual. Both sites carry the reasoning, and reconciling the two conventions is a migration this PR is not the place for.
The keys list gained typed error attribution while the Key Info screen kept ending a restore on any MessageType::Error, so the two screens hosting the same identity-scoped offer disagreed about whether that restore had ended. An unrelated task's failure still re-armed Restore on one of them. Attribution moves onto LegacyRecoveryState::owns_error, which already knows the identity it belongs to, and both screens call it from display_task_error. One implementation, so the two hosts cannot drift; returning false keeps AppState's banner, because the user still has to see the error. Key Info keeps its own blanket handling for the separate protection-migration gate, which is a different state machine with a different in-flight guard.
…e everywhere Two reviewers found the same hole in the two-candidate held-state lookup. It derived its fallback target by looking key_id up in the main identity's key set even while checking a voter-identity row, and KeyStorage::has only proves a slot is filled, not whose material fills it. A main identity holding a voting key at id N alongside a linked voting identity holding its own key at id N would report the main key as held on the strength of the other key's private half — and hand that material to Key Info. The loop already has the key, so the fallback comes from key.purpose(), and each candidate now has to hold this exact public key. Reverting to slot presence fails the new test. AC-6's third surface: the Key Info page still named its key by raw Debug purpose, so the list and the restore offer agreed with each other and not with the page they open. It now takes the same identity-scoped vocabulary, with the raw purpose kept as Expert detail rather than dropped. The CHANGELOG's known-limitation note said what the app does internally and left the reader nothing to do about it. It now names the situation, what can go wrong, and the check that resolves it — and drops the claim that saving uses "the older of the two places", which is not what makes saving risky. The collision with a same-numbered key is.
… its purpose implies A key's private half lives in a BTreeMap keyed by (PrivateKeyTarget, KeyID), and the target was derived from the key's Purpose: every VOTING key to the voter identity, everything else to the main one. That derivation cannot express a voting-purpose key filed on the main identity, which is a supported shape -- masternode_key_presence reads it as voting readiness on its own, and load_identity, the authoritative loader, files a main-identity key under PrivateKeyOnMainIdentity whatever its purpose. So for that key sign and can_sign_with looked in a store it was never filed under: the app accepted the key, saved it, showed it as held, and no signing path could ever find it. Replaced by asking the store instead of the purpose. KeyStorage::candidates probes each target at the key's own id and keeps only entries whose stored public-key data matches the requested key -- three BTreeMap probes, not a scan. Matching on the id alone is what let a lookup land on a different key: the voter and main id spaces overlap, so id 0 names two keys on a masternode. resolve_private_key_bytes now takes the public key and discovers the placement, returning the first that actually YIELDS BYTES rather than the first that matches. A vault placeholder whose secret is gone can sit beside a live entry for the same key; stopping at the first match would report such a key unusable with its bytes one probe away. With nothing to fall through to, the dead placement still surfaces its own typed error, so "the vault is not open" never degrades into "you never had that key". Discovering the placement also fixes the vault label. The map key and the label identity_key_priv.<m|v|o>.<key_id> are one composite address, so a caller that passed a target could name a label the bytes were never stored under. With this signature it cannot pass one at all. This is placement, not derivation: an AtWalletDerivationPath entry can be correctly matched here and still carry a stale path, which is what the ECDSA_HASH160 recovery scan exists for. That scan is untouched. QualifiedIdentity::placement_of answers the other question -- where a private half SHOULD go -- from the identity's own on-chain lists, for the write path only. Unknown is a real state, not a default: a locally-added key is on no list until its state transition is broadcast. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The restore offer is hosted by three screens — the keys list, Key Info, and masternode detail — and each names the keys it offers to bring back. Two of them were scoped to the identity's vocabulary; Key Info was not. A plain user's transfer key was therefore offered as a "Payout address key" there, asserting the user owns a masternode, and contradicting the keys list one screen away, which calls the same key a "Transfer key". Someone deciding whether to restore a key should not have to work out that the two names are the same key. The literal fix is one argument, but a third site forgetting the same call is a design problem, so the footgun goes with it: `vocabulary` moves from a builder option defaulting to the masternode wording to a required constructor argument. Omitting it is now a compile error rather than a plausible-looking wrong label, and a fourth host cannot inherit masternode words by accident. A required argument catches a *missing* vocabulary, not a *wrong* one, which is exactly what happened here — so the guard is at the host: the new test drives Key Info through the route the user takes, arms the offer, and reads the words off the rendered offer. Reinstating the wrong vocabulary fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stored copy of a public key is a snapshot taken when its private half was saved. A Platform identity public key is immutable once added with one exception — disabling rewrites `disabled_at` — so full-struct equality between the snapshot and the live key stopped matching the moment a key was disabled or rotated, and the row reported a key this device demonstrably holds as missing. Same symptom as the voter-identity lookup bug, different trigger. It fails safe, understating possession rather than over-, which is why it survived the earlier fix. `same_key` therefore excludes the one field that legitimately moves, and nothing else. Comparing just the id and the key material would also fix this and reopen a worse hole in the other direction: `id` is already the lookup key, and a main identity's voting key and a linked voter identity's key can carry identical `data`, leaving `purpose` as the only thing telling them apart. Conflating those reports the main identity's key as held on the strength of a different key's private half and hands that material to Key Info — the exact defect this lookup exists to prevent. Verified both ways: the narrow id+data comparison passes the new disabled-key test and fails `a_same_numbered_key_on_the_voter_identity_is_not_mistaken_for_this_one`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t through screens KeyInfoScreen carried an Option<PrivateKeyTarget> that a caller could supply and fell back to the purpose derivation when none did. One caller of fifteen supplied it, and Screen::screen_type dropped it on every round trip, so even that one lost it. With the resolver in place the field has nothing left to do: the screen asks the identity where the key is each time it needs to know, so there is no state to thread and no caller left to forget it. target() now prefers an existing placement -- that is where the material is -- and falls back to the identity's own on-chain lists for where a new private half belongs. Both agree with where the resolver will look. It returns Option, because a key on none of this identity's lists with nothing held for it cannot be placed honestly; the write, display and sign paths say so in the user's words rather than guessing a store. The write reuses an existing placement, so re-entering a key overwrites it instead of growing a second copy under another store. The delete removes every placement holding that key, selected on key material, so a duplicate cannot survive the removal the user asked for and the removal cannot land on a different key that merely shares the id. keys_screen's filed_at probed the structural target then the purpose-derived one using KeyStorage::has, which matches on key id alone. That is the collision in read form: on a masternode the voter and main id spaces overlap, so a bare id hit could report an unheld key as held against another key's material. Replaced by the same resolver as everywhere else, which selects on material. With every reader migrated, impl From<Purpose> for PrivateKeyTarget is deleted. It compiled away with no fallout, which is the evidence that the derivation was fully contained -- and its absence is what stops a second derivation being reintroduced by accident. Also pins the real v0.9.3 blob's keys as reachable, not merely decodable. The existing test proves the bytes survive the bincode version gap; this one proves an upgraded masternode can still sign with its owner key and still vote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…list gave it The detail view's key rows already knew where each key's private half was — they looked there to decide whether it was held — but dropped that on the way to `KeyInfoScreen`, which then re-derived the location from the key's purpose. The two disagree for a key filed on the voter identity whose purpose is not `VOTING`, and `role_label_and_tip` exists precisely because that shape occurs: a key on a voter identity is the node's voting key whatever its purpose field says. So the list said "Voting key" and the page one click later said "Authentication key", about one key. The same wrong target drives the page's own re-read, so it also reported a key the device demonstrably holds as missing — the identity-path defect, on the masternode path. Forwarding the row's target closes both. The new test drives the real route (card → detail → key row → Key Info) rather than the pieces, since this drifted where nothing crossed the two surfaces; reverting the fix fails it on the name assertion. Also corrects a comment falsified by this branch: the identity keys list is no longer "the static read-only KeysScreen table". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e bypassed With placement resolved from key material rather than derived, all of the correctness lives in the resolver. A caller reaching past it into the map does not merely look wrong -- it can miss a key that is present and report a saved key as absent. The field is now private, so reaching past it is a compile error instead of a review question. Direct field access is replaced by a named API that says which question it answers. candidates() is the one that finds a key. entry_at / insert_at / remove_at name a placement explicitly, for the callers that legitimately know one: a loader walking the identity list it read a key from, and legacy recovery, which is *about* the placements an old blob recorded and must not be routed through a target-blind resolver. iter / values / len / is_empty are target-blind walks. insert_if_absent replaces an `.entry().or_insert()` reach-through, and names what it is for: folding a previously-loaded record into a fresh one so a key the new load did not resupply is kept, without overwriting one it did. This is auditability, not a wall. A writer still has to choose a placement, so the point is that every such choice is now a named call a reviewer can find, rather than a map poke indistinguishable from a read. Two production readers were reaching in with a hardcoded main-identity target and are now resolved: get_selected_wallet in ui/identities, which finds the wallet a key derives from, and the withdrawal screen's wallet-unlock probe. Both would have missed a key filed anywhere else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design note explains why placement is asked of the store rather than derived from the key's purpose, why resolution returns the first placement that yields bytes rather than the first that matches, and why the reconciliation migration this problem seemed to need was designed and then withdrawn -- with the resolver permanent, moving entries buys only one convention on disk, and the bytes it would move are irreplaceable. CHANGELOG describes it as the user meets it: a voting key that was saved, shown as present, and could not sign. No docs/kv-keys.md entry and no user-story change: the fix adds no durable state and restores behaviour already catalogued rather than introducing a feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s surface `identity_keys()`'s doc comment claimed that sharing key enumeration is "what keeps the two surfaces agreeing on which keys are held". It wasn't true. Enumeration was shared; resolution was not. The identity keys list tried both filing conventions and verified the match, while the masternode detail view passed the structural target straight through — so a voting key held on the main identity but filed by purpose derivation, which is what entering one by hand produces, read as saved on the keys list and missing on the node's page. So `filed_at` and `same_key` move out of `keys_screen` to sit beside `identity_keys` as `key_filed_at`, and both surfaces call it. Enumerate alike, resolve alike. The doc comments on both now describe what the code does: the structural target is half of pairing a public key with its private material, and this is the other half. Only reads change. The two conventions still disagree on the write path; reconciling that is a migration, tracked separately. Coverage at both levels: a unit test next to the function pins all four properties of the rule (either convention found, wrong key at the same id rejected, `disabled_at` tolerated), and a kittest drives the node's real route to prove the surfaces now agree. Reverting `detail_screen` to the raw structural target fails the kittest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… silent gap `same_key` compares an identity public key field by field, deliberately skipping `disabled_at`. Written as a chain of accessor calls, a field added upstream would simply not be compared: two keys differing only in that field would compare equal, and this function decides which stored private material belongs to which key. Destructuring `IdentityPublicKeyV0` exhaustively, without `..`, moves that decision to whoever adds the field — the build stops until they say whether it identifies a key or, like `disabled_at`, only describes its state. Verified the guard bites: dropping a field from the pattern fails with E0027 "pattern does not mention field", which is exactly what an upstream addition would produce. Behaviour is unchanged, so the existing tests hold as written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The nav branch grew its own resolution rule for the same problem -- key_filed_at plus a same_key comparator in ui/masternodes, probing both filing conventions and verifying by public half. Two implementations of one rule is the shape of defect this branch exists to remove, so they are reconciled into one. Kept from there, because it is stricter than what this branch had: same_key compares every field of the public half except disabled_at. Matching on key material alone -- which candidates() did -- is wrong in a way that matters. A main identity's voting key and a linked voter identity's key can carry identical data under the same id, leaving purpose as the only thing telling them apart, so a material-only match could report one key as held on the strength of the other's private half and a delete aimed at one would have taken the other with it. same_key moves into the model beside the resolver, since it is a pure comparison on two public keys and every layer needs the same answer. Excluding disabled_at is what it exists for: that is the one field Platform lets move after a key is added, so a key disabled on chain since it was saved must still match the stored snapshot. Both properties are now pinned by tests in the model layer. key_filed_at and its UI-layer copy of the comparator are deleted; keys_screen and detail_screen both resolve through candidates(). detail_screen loses the target parameter it was threading into open_key_info -- the screen resolves for itself, so there is nothing to pass and nothing for the ScreenType round trip to drop. One distinction the merge makes explicit: naming a key and locating its material are different questions. KeyInfoScreen names through naming_target(), the structural placement, so a key is called what the list that listed it calls it; material comes from candidates(). Naming from the material's location would let one key be called two things depending on which build saved it. Both of the nav branch's Key Info naming guards pass unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing its id The delete path had no test of its own -- the resolver tests covered which placements `candidates` returns, but nothing covered the removal that iterates them, and the resolver tests exercise new code so none of them could go red first. The removal moves into `remove_held_private_key` so it can be driven directly. The test builds the shape a real install reaches with two writers: the structural loader files a main-identity VOTING key under `Main`, while an older build's paste path filed an unrelated key under `Voter` at the same id -- ids overlap between the two spaces, so id 0 names two keys on a masternode. Confirmed RED against the retired purpose-derived removal, and the failure is the one that matters: it was the *first* assertion that broke, meaning the key the user asked to delete was still on the device afterwards, while the other key's private half had been removed instead. Design note updated: `same_key`'s reasoning in §2, and the three tests the merge added to §9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…guard Their `same_key` gained an exhaustive `IdentityPublicKeyV0` destructuring so a field added upstream breaks the build instead of silently not being compared. The function had already moved to the model layer here and theirs was deleted, so the merge conflict is resolved toward the deletion and the guard is ported to where the comparator now lives. Worth having, and for a sharper reason than tidiness: this comparator decides which stored private material belongs to which key. A new upstream field that distinguishes two keys, left uncompared, would make it report a match where there is none -- the same class of defect as the purpose derivation this branch removed, arriving through a dependency bump instead of a code change. Verified the guard bites in its new home rather than assuming it carried over: dropping `read_only` from the pattern fails with E0027 "pattern does not mention field", which is what an upstream addition would produce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (33)
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. Comment |
lklimek
marked this pull request as ready for review
July 30, 2026 08:02
lklimek
added
the
claudius-review
Collaborator
|
🕓 Ready for review — next in queue (commit 9c99fe6) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: approve with conditions — 4 MEDIUM, 0 HIGH, 0 CRITICAL
Three specialists went at this in parallel — security (secret material, vault/AAD, legacy v0.9.3 blobs), project consistency (CLAUDE.md conventions, design-doc and CHANGELOG accuracy, structure), and adversarial QA (correctness plus a test-coverage audit). They produced 19 raw findings; one duplicate pair merged, leaving 18: four MEDIUM, fourteen LOW, nothing higher.
I will say the uncomfortable thing first: the core of this is right, and it is right in the way that is hard to be right. All three reviewers independently tried to break the disabled_at exclusion in same_key and all three failed, because every other discriminating field is still compared and immutable once a key is added. The security reviewer walked every writer of the vault label end to end and confirmed map key ≡ vault label throughout — so the wallet_id ‖ label AAD binding genuinely cannot be broken by re-placement, and shipping without a migration is the correct call, not a shortcut. PROBE_ORDER makes resolution deterministic via three explicit BTreeMap::gets rather than a scan. The exhaustive no-.. destructure of IdentityPublicKeyV0 turns an upstream field addition into a compile error, which is the best single decision in the diff. The retired purpose derivation has zero residual callers repo-wide. All fourteen test names in the design record exist and — per the QA audit — assert real behaviour rather than echoing the implementation, including the v0.9.3 fixture resolving both carried keys through the live resolver instead of merely decoding them.
What did not survive is the completeness of the claimed consolidation, and the changelog.
Before merge (2)
| Finding | |
|---|---|
| CALL-002 | first_protectable_key (ui/masternodes/detail_screen.rs:691-702) is the sixth of six UI resolution sites and the only one still probing a structural (target, key_id) slot. The PR body and merge commit c95f067 both claim detail_screen resolves through candidates(); it does at 729-738 and does not at 691-702. It reproduces the exact "held but invisible" defect this PR exists to remove, on the masternode detail screen, and clones raw key bytes every frame doing it — against an explicit warning comment in the sibling keys_screen.rs. Two-line fix. |
| PROJ-001 | CHANGELOG.md announces this fix at 87-98 and, six lines later in the same ### Fixed section, still publishes the workaround plus "This will be closed by the in-progress key-placement resolution fix." This PR is that fix. The entry was appended, never reconciled. |
Track, don't block (2)
SEC-001 — remove_held_private_key drops every map placement and no vault secret. Your design.md §8 discloses this; what it does not say is that clear_identity_vault_keys builds its delete set from the blob's keys_set(), so the orphan is not swept even by deleting the whole identity. That is worse than "bytes remain with nothing pointing at them" — there is no path that ever removes them. Please give it an issue rather than a footnote.
CALL-001 — WalletTask::SignMessageWithIdentityKey and DeriveIdentityKeyForDisplay still read the vault at a caller-supplied placement with no material check and no verification that the fetched bytes derive the requested public key. Also disclosed in §8. The invariant currently holds by call-site discipline rather than by construction — which is the property the rest of this PR was written to eliminate. At minimum, qualify the rustdoc that asserts it unconditionally.
Fourteen LOW findings
Not posted inline. Highlights: placement_of hand-rolls a second id+data comparator twenty lines from the one that argues id+data "opens a worse hole" (CODE-001); KeyPlacement::Ambiguous and Unknown both collapse to None, so a key legitimately on two lists is told it "does not belong to this identity" (QA-001); legacy recovery's eligibility check ignores four of the fields same_key compares (SEC-002); sign_ecdsa_local panics on a corrupt stored scalar (SEC-004); and PrivateKeyTarget is three unit variants without Copy, cloning inside the hot probe loop the design doc advertises as allocation-free (RUST-002). Full detail in the report.
Verification caveat — read this before trusting the above
This review ran in a sandbox where the Bash tool was entirely unavailable — no cargo build, no cargo test, no cargo clippy, no git. Every finding here is statically derived from reading the head tree and diffing against the base revision through the GitHub API. Your claim that the suite and clippy are clean was not independently re-executed; repository CI is the backstop for that. Three LOW findings carry an explicit UNVERIFIED: prefix where pre-existence could not be established without git. I would rather tell you that than let you assume a green tick I did not earn.
Fix the two before-merge items and this is a genuinely good piece of work — which, I assure you, is not a sentence I hand out freely.
🤖 Reviewed by Claudius the Magnificent AI Agent
Contributor
There was a problem hiding this comment.
🟠 MEDIUM · blocking — first_protectable_key (lines 691-702) never got the memo about the consolidation.
The PR body says it "consolidated with equivalent resolution logic the base branch had independently built at the UI layer", and merge commit c95f067 states outright: "keys_screen and detail_screen both resolve through candidates()". I went and counted. Five of six UI resolution sites do exactly that — keys_screen.rs:215, withdraw_screen.rs:496, ui/identities/mod.rs:85-88, key_info_screen.rs:970/995/1351, and detail_screen.rs:729-738. Delightful.
Then there is this one, sitting in the same file as one of the compliant five, still wearing the pre-fix shape verbatim:
fn first_protectable_key(&self) -> Option<(PrivateKeyTarget, IdentityPublicKey)> {
identity_keys(&self.identity)
.into_iter()
.find(|(target, key)| {
self.identity
.private_keys
.get_cloned_private_key_data_and_wallet_info(&(target.clone(), key.id()))
.is_some()
})
}Take the structural target identity_keys paired the key with, build a (target, key_id) map key, treat a raw slot hit as "held". No candidates, no same_key. This reproduces both defect classes the PR exists to eliminate:
- Misses a key that is held. A main-identity
VOTINGkey filed underPrivateKeyOnVoterIdentityby an older build — precisely the shapea_voting_key_an_older_build_filed_under_voter_stays_findablepins as findable — is invisible here, becauseidentity_keyspairs it withMain. The "Add password protection…" CTA at 657-662 is then never offered for a key the device demonstrably holds. On the masternode detail screen. Which is, I gently note, the screen masternode operators actually use. - Matches a key that is not the one asked about. Slot probes cannot distinguish a rotated or superseded key at the same id.
entry_at's own doc comment says so: "Prefercandidatesfor the latter: this cannot tell a key from a different one sharing its id."
There is a bonus, free of charge: this is a presence check that calls get_cloned_private_key_data_and_wallet_info, cloning PrivateKeyData — for a Clear/AlwaysClear entry that copies the raw 32-byte secret. It runs inside the render body at line 658, i.e. every frame. Meanwhile keys_screen.rs:211-214 carries a comment explicitly warning against this exact pattern ("cloning the entry copies raw key bytes out of the vault unscrubbed, and this runs every frame for every key") and uses candidates(...).next().is_some() instead. Two sibling surfaces, opposite decisions, same repository.
The fix is two lines and deletes more than it adds:
identity_keys(&self.identity)
.into_iter()
.find(|(_, key)| self.identity.private_keys.candidates(key).next().is_some())Drops the structural target, drops the per-frame secret clone, makes the sixth site agree with the other five. If you think the returned PrivateKeyTarget is needed — line 658 binds it as _target and throws it away.
Collaborator
Author
There was a problem hiding this comment.
Confirmed still present at HEAD 001b3d5c. first_protectable_key (src/ui/masternodes/detail_screen.rs:691-702) still does the structural (target, key_id) probe with get_cloned_private_key_data_and_wallet_info — unchanged. Notably the sibling function in the same file, open_key_info_with_mode (lines 729-738), already uses candidates(key).next(), so 6 of 7 UI resolution sites are consolidated and this one was missed. Not resolved.
Contributor
There was a problem hiding this comment.
🟠 MEDIUM · blocking — the changelog now argues with itself, inside a single section.
Lines 87-98 add "A saved voting key can now actually sign" to ## [Unreleased] → ### Fixed. Six lines further down, still in the same ### Fixed section, the pre-existing entry at 125-133 continues to advise:
One known limitation, for a voting key stored on the identity itself rather than on a separate voting identity: … saving or removing one by hand can affect a voting key of the same number on a linked voting identity, and removing it may leave the original in place. So until then, after saving or removing a voting key on an identity like that, open the keys list and check that each key still reads as you expect, and re-enter any key that should be saved but no longer is. This will be closed by the in-progress key-placement resolution fix.
This PR is the in-progress key-placement resolution fix. remove_held_private_key now removes every placement candidates selects, and removing_one_key_leaves_a_different_key_sharing_its_id_alone pins it. get_commit on 5f82b74 confirms CHANGELOG.md gained 13 lines and removed exactly zero — the new entry was appended, the obsolete one was never reconciled.
So the release notes will simultaneously tell a masternode operator the bug is fixed and instruct them to hand-audit every key after every save and remove, and promise a future fix that shipped in the same release. Three mutually incompatible statements, one section. The PR checklist ticks "CHANGELOG.md updated", which is true in the narrowest possible sense.
Fix: delete the "One known limitation …" paragraph (125-133), or reduce it to the residual that genuinely survives. Per your own design.md §8 that residual is the orphaned vault secret on delete — not the wrong-key delete, which this PR actually closed. Whatever you keep, the sentence "This will be closed by the in-progress key-placement resolution fix." must not survive in a section that announces that fix.
Collaborator
Author
There was a problem hiding this comment.
Confirmed still present at HEAD 001b3d5c. CHANGELOG.md's ### Fixed section still carries both entries: the new "A saved voting key can now actually sign" bullet, and — a few lines later, inside the "An identity's keys are reachable again" bullet — the stale "known limitation ... This will be closed by the in-progress key-placement resolution fix" paragraph, which now self-referentially describes this same PR as still pending. Not reconciled.
Contributor
There was a problem hiding this comment.
🟠 MEDIUM · non-blocking — the rustdoc on resolve_private_key_bytes (mod.rs:669-672) states an invariant that two live vault paths do not honour.
The doc is admirably confident:
"a caller cannot be trusted to pass a target that agrees with the blob — and with this signature it cannot pass one at all"
Correct for this function. Not correct for the vault. WalletTask::SignMessageWithIdentityKey and WalletTask::DeriveIdentityKeyForDisplay both still carry a target: PrivateKeyTarget in the task payload and hand it straight to AppContext::with_identity_secret_key (src/backend_task/wallet/mod.rs:107-135), which builds SecretScope::IdentityKey { identity_id, target, key_id } and reads the vault at exactly that label:
async fn with_identity_secret_key<T>(
self: &Arc<Self>,
identity_id: Identifier,
target: PrivateKeyTarget,
key_id: KeyID,
f: impl FnOnce(SecretKey) -> Result<T, TaskError>,
) -> Result<T, TaskError> {No candidates() probe. No same_key. And — the part that actually matters — no check that the fetched bytes derive the public key that was requested before a WIF is exported to the UI or a signature is emitted under that key's name. Nothing downstream compares the derived public key against self.key, so a mismatch is entirely silent.
I want to be fair here: your design.md §8 defers precisely this ("Validating the signing path's vault-resolved key against the requested public key, deferred separately"). It is a disclosed gap, not an oversight, and I am not calling it blocking. I am raising it because (a) the resolver's rustdoc asserts the invariant unconditionally while two vault-reaching paths sit outside it, and (b) the orphaned vault labels from the delete path are exactly the material a named-placement read can still reach.
Today the caller happens to be KeyInfoScreen::target(), whose first branch is candidates().next(), so in the UI flow I could trace the label does agree with the blob. Which is to say: the invariant currently holds by call-site discipline, not by construction — the precise property the rest of this PR was written to eliminate. That is an uncomfortable place to leave the one path that hands out raw key material.
Two fixes, either sufficient:
- Verify after the fetch. Inside
with_identity_secret_key, derive the public key from the resolved secret and compare it against the requestedIdentityPublicKey(KeyType::public_key_data_from_private_key_data— the same checkkey_exclusionandvalidate_private_key_bytesalready perform), returningTaskError::IdentityKeyMalformedon mismatch. This is the §8 deferral and it is a handful of lines. - Remove the caller-named placement. Carry the
IdentityPublicKeyin the twoWalletTaskvariants instead ofPrivateKeyTargetand let the tasks callresolve_private_key_bytes. This also hands them the fall-through behaviour they currently lack.
Prefer (1) now, (2) as the end state. At absolute minimum, qualify the rustdoc so it stops claiming an invariant the codebase does not yet enforce.
Collaborator
Author
There was a problem hiding this comment.
Non-blocking, tracked per design.md §8 — still accurate at HEAD 001b3d5c. WalletTask::SignMessageWithIdentityKey / DeriveIdentityKeyForDisplay (src/backend_task/wallet/mod.rs:188-204, handlers in derive_identity_key_for_display.rs:22-42 / sign_message_with_identity_key.rs:21-47) still carry a caller-supplied target passed straight to with_identity_secret_key with no post-fetch verification against the requested public key. Today's only caller (key_info_screen.rs:774) sources target via candidates(), so the invariant holds by call-site discipline, not construction — as noted. Rustdoc on resolve_private_key_bytes remains unqualified. Leaving open per the disclosed deferral; no action required for this PR.
Contributor
There was a problem hiding this comment.
🟠 MEDIUM · non-blocking — remove_held_private_key (lines 1346-1358) drops every map placement and not one vault secret, and the orphan it leaves cannot be swept even by deleting the whole identity.
fn remove_held_private_key(&mut self) -> Result<(), TaskError> {
self.private_key_data = None;
for placement in self.identity.private_keys.candidates(&self.key).collect::<Vec<_>>() {
self.identity.private_keys.remove_at(&placement);
}
self.app_context.update_local_qualified_identity(&self.identity)
}IdentityKeyView::delete is never called. The raw 32 bytes stay in the vault under identity_key_priv.<m|v|o>.<key_id> with nothing in the blob pointing at them.
Your design.md §8 discloses this, and I respect that it does. Three things it does not say, all verifiable from the head tree:
- The orphan is unreachable by the only cleanup that exists.
clear_identity_vault_keys(src/context/identity_db.rs:1063-1077) builds its delete set from the stored blob'skeys_set(). Once the map entry is gone the label is no longer enumerable — so deleting the entire identity afterwards does not purge it. The secret outlives the identity record. That is a materially worse story than "bytes remain on disk with nothing pointing at them"; there is now no path that ever removes them. - The removal got wider. Routing through
candidates()strands up to three labels per delete, where the retired purpose-derivation stranded one. - This PR makes the gap newly reachable. The §3 fall-through shape you introduce — a live
Clearentry beside a deadInVaultplaceholder for the same key — means removing the key on screen also drops the map entry for theInVault-backed placement, at which point no(target, key_id)pointer to that vault object survives anywhere. (Both the security and QA reviewers landed on this independently.)
There is also a secondary wedge on a Tier-2 identity: the orphaned Protected label survives the removal, re-entering the same key by WIF inserts Clear, encode_identity_blob_vault_first then finds the orphan via find_protected_identity_key_scope and returns IdentityKeyProtectionDowngrade — which this screen renders at 1128-1134 as "The private key could not be saved. Check available disk space and try again." An unresolvable state, paired with a remedy that has nothing to do with the actual problem. Per CLAUDE.md's error rules, every message must give the user something they can act on themselves; buying a bigger SSD will not help here.
Recommendation (follow-up, not a merge blocker — but please give it an issue rather than a design-doc footnote): delete the vault secret for every placement removed before dropping the map entry, mirroring encode_identity_blob_vault_first's vault-first ordering. A vault-delete failure must abort the removal rather than persist a record claiming the key is gone. Separately, make clear_identity_vault_keys best-effort over the identity scope instead of over the blob's keys_set(), so already-orphaned labels are still swept on identity deletion.
The user-facing stake is worth stating plainly: for a masternode owner or voting key — irreplaceable material with no seed to regenerate from — "Remove private key from DET" currently means "stop showing it to me", not "remove it". A stolen laptop or a vault-file backup still yields the key.
Collaborator
Author
There was a problem hiding this comment.
Non-blocking, tracked per design.md §8 — still accurate at HEAD 001b3d5c. remove_held_private_key (key_info_screen.rs:1346-1358) only evicts map placements via candidates() + remove_at; IdentityKeyView::delete (identity_key_store.rs:202) has zero call sites anywhere in src/ui/ or src/backend_task/. clear_identity_vault_keys (identity_db.rs:1063-1077) builds its delete set from the current stored blob's keys_set(), which no longer contains an already-evicted placement, so the orphaned vault secret survives even full-identity deletion. Leaving open per the disclosed deferral; no action required for this PR.
Contributor
github-actions
Bot
removed
the
claudius-review
…y other surface first_protectable_key still probed a raw structural (target, key_id) slot instead of going through candidates(), the one resolution rule the rest of this PR consolidated every other UI site onto. It could miss a key filed under a non-structural placement (e.g. a main-identity voting key an older build filed under the voter placement) and cloned raw key bytes out of the vault every frame doing it. Delegates to candidates().next().is_some() like its sibling open_key_info_with_mode a few lines down. Presence-only, so no secret clone.
…navigation # Conflicts: # CHANGELOG.md # docs/ai-design/2026-07-28-legacy-identity-recovery/design.md # docs/user-stories.md # src/backend_task/identity/recover_legacy_keys.rs # src/model/legacy_recovery.rs # src/ui/components/legacy_recovery_section.rs # src/ui/identities/keys/key_info_screen.rs # src/ui/masternodes/detail_screen.rs # src/ui/masternodes/list_screen.rs # src/ui/masternodes/mod.rs # src/ui/state/legacy_recovery.rs # tests/kittest/legacy_recovery_section.rs
4 tasks
…e keys whole
Three defects a review found on the Key Info navigation work.
Key Info's arrival re-read hung off `refresh_on_arrival`, which `AppState`
dispatches only to root screens. This screen is always pushed onto the screen
stack, so the reload, the protection-status reset and the offer re-arm were
unreachable: a write landing off-screen was erased by the next ordinary key
edit, which persists the whole identity clone the screen opened with. The body
moves onto `refresh`, which does reach a pushed screen; `refresh_on_arrival`
delegates there by default, so both hooks still run it. The regression test
moves to kittest and drives the app's own `TaskResult::Refresh` dispatch, since
hand-calling the hook passed against the bug.
The offer to restore stranded keys was hand-copied across its three hosts:
render, result routing, error routing, dispatch. The render chain becomes
`legacy_recovery_section::host_offer` and the attribution rules become
`LegacyRecoveryState::absorb_result` / `absorb_error`, leaving each screen only
its own side-effects. A shared rule applied to two hosts of three is what caused
an earlier bug here; a fourth host now cannot repeat it.
Key role labels were assembled from fragments — a bare role word, an English
noun, a state, a positional key id — and two surfaces interpolated a raw
`Purpose` through `Debug`. `role_label_and_tip` now returns whole `&'static str`
captions keyed on vocabulary, purpose and retired state, so composing one at a
callsite is impossible rather than discouraged, and matches every `Purpose` by
name so a variant added upstream breaks this build instead of reaching a user as
`{:?}`. The raw purpose keeps its place on Key Info as its own Expert field.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The catalog entry for viewing identity keys still described criteria that predate a reachable route into a key's own page — this branch is what first gives `KeysScreen` a way into `KeyInfoScreen` at all. The real criteria had landed under IDN-020, a legacy-migration story, and IDN-008 also overstated the Everyday view: the new list gates key id, purpose, security level, type and read-only behind the Expert view. Restates the criteria as what ships — reachable from the identity's Settings → Advanced without changing interface mode, every key openable whether or not the device holds it, role and held state in words, on-chain specifics as Expert detail — and cross-references IDN-020 for the restore offer rather than duplicating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6 tasks
…o fix/889-key-target-derivation-consistency # Conflicts: # src/ui/identities/keys/key_info_screen.rs
The merge of fix/889-key-info-navigation brought in test fixtures written against KeyStorage's pre-privatization field (private_keys.private_keys), which this branch made private so callers cannot bypass the resolver (design.md §7). No textual conflict flagged it since the two branches touched different files; cargo build --all-features --tests caught it. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…t-derivation-consistency # Conflicts: # CHANGELOG.md # src/ui/identities/keys/key_info_screen.rs # src/ui/identities/keys/keys_screen.rs # src/ui/masternodes/detail_screen.rs # src/ui/masternodes/mod.rs # tests/kittest/key_info_screen.rs # tests/kittest/keys_screen.rs # tests/kittest/masternode_tab.rs
lklimek
enabled auto-merge (squash)
July 30, 2026 17:11
orchardpaytl
pushed a commit
to orchardpaytl/orchardpay
that referenced
this pull request
Jul 30, 2026
… by deriving it from purpose (dashpay#889) (dashpay#946) * feat(model): add the legacy identity recovery plan and additive merge Issue #889: a v0.9.3 identity that was already partially loaded before the upgrade keeps its remaining keys stranded in `data.db`, because the migration skips a present identity wholesale. That rule is correct — three earlier reconcile heuristics each failed by inferring user intent from record shape. `compute_recovery_plan` only lists what the legacy record holds and the modern one does not, keyed on the `(target, key_id)` map slot and on `None` associations; it never decides whether an item should come back. That decision travels back in as an item-level allowlist, so the absence-versus-removal ambiguity lands with the only party holding the provenance. `apply_recovery_plan` is additive by construction, not by discipline: it starts from the fresh modern record, inserts with `or_insert` so the modern copy wins every collision, recomputes candidacy rather than trusting the allowlist, and has no write path at all for alias, status, the dpp identity, DPNS names, or the wallet link. A voting key drags its voter-identity link along, since one without the other cannot vote. M1-M10 from the design doc's test matrix. M2/M5/M6/M9/M10 were confirmed RED against a naive legacy-wins merge before this implementation landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(database): read one legacy identity row through the shared row decoder Issue #889: the recovery flow needs one identity out of the preserved legacy `data.db`, not the whole table. A second hand-written reader would be free to drift from the importer's idea of which rows are readable — and then recovery could merge from a record the migration never considered importable, or miss one it did. So the per-row decode moves into `decode_identity_row` and both readers call it, along with one shared `SELECT` that binds the same pair of accepted network spellings. `read_identity_row` adds only the `id` predicate on top. `LegacyIdentityLookup` keeps "nothing to recover" and "the row will not decode" apart. An absent table, an absent row, an observed-identity cache row and a NULL blob are all `Absent` — the ordinary answer. Only a genuinely corrupt row is `Unreadable`, so no caller can mistake corruption for an empty record and merge against it. `open_legacy_read_only` is hoisted next to `database/mod.rs`'s existing one, so `SQLITE_OPEN_READ_ONLY` is a property of the one function that opens the file rather than of each caller remembering the flag. The importer's 12 existing row-decode tests are the regression net for the extraction; 8 new tests cover the lookup, including one asserting both readers decode the same row identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(identity): restore keys stranded in the previous version's saved data Issue #889: an identity that was only partially loaded before the v1.0 upgrade keeps its remaining keys in `data.db`, reachable by nothing — the load form rejects the duplicate ProTxHash, and the per-key screen wants a WIF the user may no longer hold. This is the way in, and it is entirely user-driven. `CheckLegacyRecovery` lists; `RecoverLegacyIdentityData` writes. Detection is read-only, offline and side-effect-free, so a screen can dispatch it on arrival and hide the affordance when the plan comes back empty. Candidacy is recomputed inside the executing task rather than trusted from the caller: only `recomputed-candidates ∩ approved` is merged, so an approval that went stale since the preview is reported rather than acted on, and an item the user never approved is never restored even when it is missing. An empty allowlist is refused, not widened into "restore everything". The protection-downgrade trip that sank an earlier reconcile attempt is unreachable here by branch condition, not by care: the flow branches on the same predicate the at-rest guard evaluates. On a protected identity the password is verified up front through the shipped prompt, the resident-plaintext preflight runs before any vault write, and every merged key is sealed under that password before the record is persisted — so the encoder never sees plaintext on a protected identity. On a keyless identity there is no protected key for the guard to protect. Cancel, wrong password and headless all fail closed with the stored record byte-identical. Exactly one record write, so the merge either lands whole or changes nothing. No new persisted state: eligibility recomputes from the two existing stores, so a re-run is a no-op and `data.db` — opened read-only — stays a repeatable recovery source. `reject_resident_identity_plaintext` widens to `pub(super)`; it now guards both seal boundaries. Two test-only readers expose the at-rest blob and the wallet link, which the hydrated record cannot show. B1-B12 from the design doc's test matrix, on the offline wired `AppContext` harness, plus the excluded-key and empty-allowlist cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * feat(masternodes): offer the previous version's stranded keys on the node page Issue #889: the restore path exists but nothing reaches it. The node detail page is the canonical surface — its operator is the one holding a node that cannot vote because its voting key stayed behind in the previous version's saved data. The offer is passive and contextual, never a launch-time nag. On opening a node the view dispatches the read-only detection task once, and only on an install that actually has a previous-version database; the section renders in the Keys area only when detection found something, and retires itself once a restore lands, because eligibility recomputes from the two stores every time. For a node with no voting key that becomes the primary remedy: the missing-voter message points at restoring, which asks nothing of an operator who no longer has the key, and the existing WIF prompt keeps its place as the fallback for one who does. `LegacyRecoveryState` (`ui/state/`) owns the fetch state — dispatch-once, in-flight, offered, retryable — and `LegacyRecoverySection` (`ui/components/`) renders it, per the placement rule's render/no-render split. A failed restore returns to its offer so a mistyped identity password can be corrected and retried; a failed detection does not re-ask. A preview result is routed into the open detail view rather than triggering the list screen's reload-and-reopen, which would rebuild the view, re-dispatch its check and never settle. A completed restore does reload, since the store changed, and the rebuilt view's fresh check finds nothing left. `RecoveryItemDescriptor::is_voting_key` factors out the rule `label` already applied, so "is this the voting key" has one definition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * feat(identities): offer stranded legacy keys on the Key Info screen Issue #889: the node page covers the masternode case, but a `User` identity that was partially loaded before the upgrade has no node page. The Key Info screen is the issue's own suggested location and the one surface every identity type reaches — from the Identities list for a user identity, and through Manage keys for a node. The offer is identity-scoped rather than key-scoped, so it renders whatever the opened key's own state is: what it lists are precisely the keys the identity does *not* hold, which no per-key view could show. Both surfaces share `LegacyRecoveryState` and `LegacyRecoverySection`, so detection, the allowlist and the copy have one definition each. Dispatch follows this screen's established queue-then-drain pattern, alongside the protect/unprotect ones. The check and a restore can never contend for the frame's single `AppAction`: the check goes out once on arrival, and a restore only after a press. A completed restore re-probes the protection status, since the restored keys have just landed in the vault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * docs(identity): record the legacy-key recovery flow and close the migration gap Issue #889: the migration design's §7 named this limitation and deferred it to "a dedicated, provenance-aware flow". That flow now exists, so the limitation gets its resolution rather than staying an open pointer to an issue — with the one clarification a reader of that section needs: the importer itself did not change, skip-if-present still stands, and nothing about recovery runs at migration or launch time. The design record moves into `docs/ai-design/` under this repo's dated-directory convention, so the cross-reference resolves inside the repo. U1 covers the widget from the outside: an empty plan renders nothing, items are named by role with the voter link folded into its voting key, pressing Restore approves exactly the previewed set, an in-flight restore has no button to press twice, unrestorable keys are listed with no restore offered, and an install with no previous-version data never even asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(model): only offer legacy keys that still correspond to the identity The plan admitted any legacy key the modern record happened not to hold, keyed on the map slot alone. Nothing checked that the saved private half derived the public half it was stored with, that the public half was still a live key of the identity, or that a wallet-derivation reference named a wallet this install holds — the exact checks the manual "type the WIF" path this flow replaces already enforces. That matters because `masternode_key_presence` reports a role as held from the record alone. Restoring a key the node rotated away from flipped the role to "present", retiring both the recovery offer and the missing-voter remedy, and the operator found out from a rejected transaction. The voter link alone did the same thing with no key behind it at all. Failing candidates now go to `excluded` with their own reason and are never restorable. `AppliedRecovery` also reports that `excluded` list, so an approval naming something that was never restorable is no longer also reported as a stale approval ("already back in place" — the opposite answer), and the caller stops recomputing the same plan to read one field off it. Whatever plaintext the legacy record still holds after a merge is zeroized instead of being dropped intact. The item labels and exclusion copy move out of `model/` into the UI, where they now delegate to the shipped `role_label_and_tip` vocabulary rather than a second mapping that disagreed with it on the same screen ("Payout key" next to "Payout address key"), and two rows in one role are told apart by key id instead of rendering identically. Test fixtures move onto genuinely derived key pairs with the chain's public keys published on the record — a correspondence check that passes against placeholder key material verifies nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(identity): stop holding the storage guard across the password prompt `recover_legacy_identity_data` took `migration_run` for the whole async span, prompt included. The delete path it copied that pattern from is fully synchronous and holds the mutex for microseconds; this held it for as long as a modal sat in front of the user. Two independent reviewers reproduced the fallout: removing an *unrelated* identity failed with "storage is still being updated" when no migration was running, and `finish_unwire`'s awaiting acquire could park indefinitely. The flow is now a preflight and one fully synchronous critical section. The preflight reads, dry-runs the merge to see whether anything would be restored at all, decides whether a password is needed, and prompts — holding nothing. The write section then re-acquires the guard, re-checks the migration state, re-reads the record, merges again and writes, all without an await in between. Re-reading is not just tidiness. The prompt window let any other writer land in between — refresh, DPNS registration, transfer, none of which take the load claim — and the write of a pre-prompt snapshot silently reverted it. It also makes the no-resurrection guarantee explicit: a delete during the prompt now surfaces as `IdentityNotFoundLocally` instead of being undone by the upsert, which until now held only as a side effect of how long the lock was kept. The at-rest protection predicate is evaluated over the merged record, the one the encoder's downgrade guard actually sees, rather than over the pre-merge record. If the identity gained password protection during the prompt the verified password no longer covers what is about to be written, so the restore stops with a typed `LegacyRecoveryIdentityChanged` rather than sealing under a password for a different state. Also: the nothing-to-recover success no longer records a `Failed` load, the preview zeroizes the stranded plaintext it decodes on every screen open instead of releasing it intact, and the "nothing restored" banner is worded to be true of both outcomes that produce it. Both concurrency tests were confirmed RED against the old lock scope, reproducing the reviewers' exact failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(ui): keep the restore offer and the record it wrote in step Three ways the two shipped surfaces disagreed with the store after a restore, and the state machine behind them had no tests at all. The Key Info screen keeps the identity clone it opened with, and its own add-key and remove-key paths persist that whole clone. This feature added a second writer to the same record reachable from the same screen: restore keys, then add or remove any key, and the pre-restore copy was written back — the restored keys gone, right after a banner said they were back. The completion now re-reads the record and the key on screen from it. The node page never hears about a restore run from the Key Info screen it pushed: the pushed screen is on top, so it takes the result. Coming back left the page offering keys already restored and warning about a voting key it now held. Arrival re-reads the node and re-arms its check; vote selections and any open prompt survive, which a blanket rebuild would have discarded. The `recovery_completed()` call in the result handler goes with it — the reload that follows re-opens the view and re-arms the check anyway, so it was mutating a view discarded three statements later. Writing the state machine's tests turned up a third one: `completed()` re-armed unconditionally, so calling it on an install with no previous-version database armed a check that must never run. Harmless until arrival started calling it; now `Unavailable` stays unavailable. The Restore button loses its ellipsis: on a keyless identity — the common case this feature exists for — it commits immediately rather than opening anything. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * docs(identity): record what the recovery flow actually shipped The committed design record was landed marked "implemented" with exactly one edit — the status line — so the repo's permanent account of this feature described a dispatch model, a progress mechanism, a module set and a gating rule that were all deliberately changed, plus the pre-review merge rules that the fix round replaced. A §10 now records all nine departures with the reasoning that produced them, rather than rewriting §1–§9 into a story nobody actually followed; the status line says which of the two is authoritative. §7's claim that `finish_unwire.rs` is untouched is reconciled with §4.2's hoist in the same document: the importer's behaviour is unchanged, the file is not. `reject_resident_identity_plaintext`'s header claimed to guard "every boundary that seals an identity's keys". It does not — the merge-load path seals without it. The header now says which boundaries it covers, and that path carries a TODO describing the gap. Fixing it is a separate issue: it is pre-existing code, unrelated to this feature. Also: user story and CHANGELOG pick up the correspondence check and the non-blocking prompt, the components catalog names the copy this feature's component now owns, and three private rustdoc blocks come back inside the internal-commentary budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(model): stop the legacy blob from vouching for its own voter key The correspondence check that keeps a rotated-away key out of the offer resolved a voter/operator-target key's identity as "the modern link, or the legacy one if there is no modern link". In the case the check exists for -- a masternode whose record carries no voter link -- that fallback read the identity snapshot out of the same unauthenticated legacy blob as the key under test, so any self-consistent pair passed. A voting key the chain retired months before the upgrade still reads as live in that snapshot, and restoring it flips the node's voting role to "present", retiring the manual remedy that would actually fix the node. reference_identity now reads the modern record only. A key on a voter or operator identity the modern record does not link to has no admissible witness and is excluded as LinkedIdentityUnverified, carrying the same "load this identity again and enter the key" remedy as its neighbours. Fetching the linked identity from Platform was rejected twice over. It would put the network in the middle of an offline, read-only preview that every screen arrival dispatches, and model/ is pure by contract. More decisively, it would not establish the property: a voter identity's id is derived from its voting key (Identifier::create_voter_identifier), so rotating the key on-chain creates a *different* voter identity rather than retiring a key on the existing one. Fetching the legacy-named voter identity would confirm the stale key against its own orphaned identity. Behaviour change worth noticing on its own: a masternode loaded from its ProTxHash alone no longer gets its voter-identity-target voting key back in one click. The key is listed as unverifiable, the voter link goes with it, the node keeps reporting the role as missing, and render_missing_voter keeps offering manual entry -- the honest outcome, since nothing available offline can tell that key from one the chain replaced. A record that does carry the voter link (a re-load keeps it) restores the key as before. Grouping follows the same rule: voter_association_is_grouped now keys on the voting role, the predicate that already decides whether the link is worth offering, so the two cannot disagree about which key the link belongs to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(identity): gate the recovery offer on legacy rows, not on data.db existing FetchState::Unavailable was documented as the state a fresh install never leaves, but no shipped install could reach it. AppState::boot_inputs opens an existing data.db read-only *or* creates one and runs the fresh-install schema ladder, so db_file_path().exists() is true for every user, and every identity or node screen dispatched a detection task against a table the ladder had just created empty. The kittest that claimed otherwise passed only because the `testing` feature substitutes an in-memory database whose path is None -- a test of the substitution, not of the gate. AppContext::has_legacy_identities now asks whether the legacy `identity` table holds a local row for this network: one SELECT EXISTS over the same filter read_identities uses (so "there is something here" and "here it is" cannot drift), false when the table does not exist at all, answered once per context and cached. A probe that errors arms the offer instead of retiring it -- the detection task reports its own typed error, while a silent false would withdraw a recovery the user's data still supports. The kittest is replaced by lib tests over a real file-backed data.db, which assert the premise the old gate missed (the file is there and the gate still says no) and cover the network scoping a file check could never have. Confirmed red against the old gate before the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(ui): re-read the key screen on arrival and attribute a finished restore Two ways a restore's result reached the wrong screen state. KeyInfoScreen had an empty refresh(), which refresh_on_arrival defaults to, so returning to a screen that missed its result never re-read the record. That screen persists the whole identity clone it opened with on every key add or remove, so the clone -- predating the restore -- was written back over the restored keys on the next edit, silently and with a successful save to show for it. It now re-reads on arrival, re-probes the protection line and re-arms its check, exactly as the masternode detail view already does. The LegacyRecoveryCompleted arm discarded identity_id, unlike the LegacyRecoveryCandidates arm directly above it, which routes through LegacyRecoveryState::offered's identity check. Results reach whichever screen is visible when they arrive, not the screen that dispatched them, so a restore dispatched from identity A's Key Info screen could land on identity B's: B showed "your keys have been restored to this identity" for a restore that was never B's, and B's own in-flight recovery state was reset under it. The attribution rule now lives once, in LegacyRecoveryState as completed_for -- the twin of offered -- so both call sites share it rather than each remembering to check. The masternodes list applies the same rule through shows_node: a completion for an identity no card and no open detail names is dropped, banner, reload and detail rebuild included. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * test(identity): pin the mid-flight protection guard against regression TaskError::LegacyRecoveryIdentityChanged is listed in persist_legacy_recovery's rustdoc and spelled out in design.md 10.6, but was the only documented error branch of that function with no test -- every sibling (IdentityLoadInProgress, WalletStorageNotReady, IdentityNotFoundLocally, LegacyIdentityUnreadable, the prompt's cancel and unavailable errors, IdentityKeyProtectionIncomplete, the deleted identity, the undecodable row) has one. B13 drives the 10.6 scenario: the dry run on a keyless identity decides no password is needed, a ProtectIdentityKeys lands before the write section re-reads the record, and the write refuses with the typed error and a byte-identical stored blob. It calls persist_legacy_recovery directly with the None the dry run produced, as B5 calls verify_recovery_password directly: a Tier-1 restore never prompts, so there is no await point between the two where a concurrent task could be made to land deterministically. The dry run is exercised first so the None under test is the one production carries. Verified the test fails when the fail-closed branch is removed, rather than merely restating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(model): stop the legacy blob from vouching for its own operator key The voter-target fix landed one half of the rule: a key on a voter or operator identity the modern record does not link to has no witness outside the legacy file, so it is excluded. The operator *link* itself was offered with no gate at all, which left the same hole open one step further out. Restoring it writes the legacy file's own claim into the modern record. The next preview then reads that claim back as the outside witness for the same file's operator keys, and every one of them becomes a candidate — self-vouching in two moves instead of one. Nothing in production ever writes `associated_operator_identity` fresh, so an offered operator link was always in that unverifiable state. An operator link the modern record does not carry is now excluded as `LinkedIdentityUnverified`, like the keys held on that identity. Also renames M8, whose name and opening sentence claimed it proved the whole legacy key set is offered while its own assertions exclude two of five items. * fix(masternodes): drop the missing-voter message that can never be shown The node page carried a second missing-voter message offering restore as the primary remedy, chosen whenever the recovery plan held a voting-role candidate. Since the voter-key check began reading the modern record only, that candidate exists only when the record already carries the voter link — and a record carrying the voter link is written in the same branch that writes the voting key, so it is never in the missing-voter state to begin with. The two conditions are mutually exclusive: no user, and no test, has ever seen that message or its companion tooltip. Gone, along with the predicate behind them. The remaining message is the honest one and still matches what the screen offers: the in-place "Add voting key" prompt right underneath it, which needs no re-load. Restoring a voting key held on a separate voter identity needs an on-chain check this flow deliberately does not make; that is issue #942, not something this branch pretends to do. * fix(ui): let the recovery section read as a whole when it can restore nothing A plan holding only unrestorable items — now the ordinary shape for a masternode loaded from its ProTxHash — opened with "Some keys ... haven't been brought across", followed by no list, no button, and then a second heading for the items it cannot bring back. A lead-in that promises an action the section never offers reads as a broken screen. That case gets its own lead-in, which introduces the list itself, so the second heading appears only when there really are two lists to keep apart. The reason an item cannot be restored also stops being hover-only. It is the single sentence that tells the user what to do instead, and a tooltip reaches neither touch nor keyboard; it is now an inline line under each item. The reason for an unverifiable linked identity is reworded to read correctly for the operator link as well as for a key held on one. * docs(identity): stop promising a voting-key restore that cannot happen The user story and the changelog both said restoring is offered as the first remedy for a node with no voting key, with typing the key in as the fallback. For the flagship case — a masternode loaded from its ProTxHash alone — that is never true: the voting key sits on a separate voting identity that only the previous version's data names, so it is listed as one that cannot be brought back and entering it by hand is the only remedy. Naming owner, voting and payout together in the same breath carried the same implication and is dropped. Both now describe what happens, and point at issue #942 for the on-chain check that would let the voting-key case be closed safely later. The design record gains the two shipped deviations behind this: §2.1's "restore first, WIF as fallback" branch removed as unreachable, and the operator link excluded as unverifiable for the same reason its keys are. * fix(identity): give the identity keys list a way into each key The identity hub replaced the legacy Identities screen's per-key popup — the only route into KeyInfoScreen that did not depend on the identity already holding a usable key — with Settings > Advanced > "Manage keys". That destination was a read-only table with no route onward, and RootScreenIdentities is deliberately out of the nav, so the popup it still hosts is unreachable. Every remaining route (transfer, withdraw, the token screens) is gated on holding a key of the kind that action needs, which is false for precisely the identities issue #889's restore offer exists to help: a user identity with stranded keys could reach its own keys only in Developer view, through a send-money screen. KeysScreen now carries the QualifiedIdentity, so it can say which keys this device holds and open KeyInfoScreen with that key's private material. One row per key, named in the shared role vocabulary, held state stated in words rather than by colour alone, ungated. The restore offer renders on the list itself, above the rows: it is scoped to the identity, and making a user pick an arbitrary key to discover an identity-level offer repeats the defect one level up. It stays on KeyInfoScreen too, which self-extinguishes and is where the masternode path lands. The screen also moves inside the app chrome (top panel, left panel, island_central_panel). That is not cosmetic: island_central_panel is the only caller of MessageBanner::show_global, so on a raw CentralPanel the restore reports nothing at all — a failed restore would be indistinguishable from a successful one. key_role_label, manage_keys_labels and the key enumeration move from detail_screen into ui::masternodes beside the vocabulary they belong to, so the two "Manage keys" surfaces cannot drift apart; the masternode kittest covering that list is the regression guard for the move. The transfer screen's no-key branch gains a "Manage keys" signpost. Its available_transfer_keys() gate is untouched — it still correctly says the identity cannot send; the branch simply stops being a dead end. * test(identity): pin the restore outcome and the key naming on the keys list The offer self-extinguishes on the next check whether the restore landed or failed, so its disappearance says nothing about the outcome. Only the banner distinguishes the two, and the banner only reaches the user because the screen is built on island_central_panel — the sole caller of MessageBanner::show_global. Cover both outcomes explicitly, and pin the render channel itself: reverting the screen to a bare CentralPanel fails the new guard, which is what makes it worth having. Also pin AC-6 across the two surfaces that name keys. The restore offer and the keys list render from different modules over different types, and nothing but their shared derivation from role_label_and_tip keeps them saying the same words; a private re-implementation on either side would read plausibly and drift in silence. Key Info remains outside that assertion: it does not retain the PrivateKeyTarget its key was opened with, so it cannot tell a voter-identity key from a main-identity one, and naming it in role words today would risk labelling a voting key as an authentication key. Threading the target through its nine call sites is the fix, and it is wider than this change. * test(identity): report both restore outcomes on the keys list in the running app The component tests prove two things separately: the screen sets a banner on a finished restore, and it renders whatever banner its context carries. Neither closes the seam between them — that the context the screen writes its outcome into is the context it renders from. A restore reporting into one context while the screen renders another would pass both and still tell the user nothing. This drives the real app to the keys list, delivers a completed restore through visible_screen_mut the way AppState does, and asserts the success text appears on the screen; then banners a task error on the app context, as AppState does for a failed restore, and asserts that appears too. The offer retires itself either way, so the banner is the only thing that distinguishes them. Reverting the screen to a bare CentralPanel fails this test, which is what makes it a guard rather than a decoration. * fix(identity): make the keys list and Key Info agree on where a key is filed SEC-001. The keys list resolves each key's PrivateKeyTarget from the identity it walked, then discarded it; KeyInfoScreen re-derived one from the key's purpose. impl From<Purpose> for PrivateKeyTarget sends every voting key to the voter identity, so for a Purpose::VOTING key filed on the main identity — a supported shape, which masternode_key_presence reads as voting readiness on its own — the two disagree. The list reports the key as held, Key Info re-reads the record on arrival, looks in a store the key was never filed under, finds nothing, and reports it missing. On the screen built to answer that question, about a key whose private half is listed one screen earlier. KeyInfoScreen now accepts the resolved target via with_target and uses it for every lookup, falling back to the old derivation when no caller supplied one, so the call sites this change does not touch keep their current behaviour. Only the keys list passes it. The same derivation still governs the pre-existing callers' reads, writes and deletes — including a delete that can land on a different key when voter and main key-id spaces overlap — which is filed separately. Also from QA: - SEC-002: the held/not-held column called get_cloned_private_key_data_and_ wallet_info, copying raw key bytes out of the vault unscrubbed, every frame for every key, to read is_some(). KeyStorage::has answers it without a copy. - QA-005: display_message ended a restore on any MessageType::Error. Results are not screen-affine, so an unrelated task's failure re-armed Restore mid-flight and it could be dispatched twice. Attribution moves to display_task_error, which receives the typed error; it returns false so the user still gets AppState's banner. Residual documented on is_recovery_error: the same variant from another task during a restore still ends it, which needs the task's identity to reach the screen. - QA-003: the completion-banner test asserted only that some banner existed. It now pins attribution instead — another identity's restore reports nothing here — since fresh_app_context drops the harness its context belongs to and the text cannot be read back at that level. The running-app test already pins the text. - QA-006/007: coverage for the transfer-screen signpost and for the Expert-only detail row in both directions. - Empty-state copy said "no keys saved on this device", which in this screen's own per-key vocabulary means held=false. An empty record is a different statement; the test that codified the wrong string is fixed with it. - QA-002: "island_central_panel is the only caller of show_global" is false — contracts_documents_screen calls it directly. Corrected where it was written down as justification. - key_role_label back to pub(crate); ui/masternodes' module header no longer implies its key vocabulary is Expert-Mode-scoped, since an Everyday identity screen depends on it. - Docs: §2.1 and §7 row 1 restored as written, per the document's own proposal-plus-deviations model, with §10.13 carrying the correction. CHANGELOG no longer claims transfer was the only route to a key's page, and no longer promises signing or password protection to an identity that holds no keys yet. * fix(identity): return from a key to a current list, in the identity's own words Track A of round 3. Leaving a key now pops with a refresh, and the keys list re-reads both halves of its state on the way back. Only re-reading the record would show a key restored from the pushed Key Info screen as held while still offering to restore it — and pressing Restore then reports there was nothing left to do. Consolidating into refresh() also retires the refresh_on_arrival override, which never ran: the framework dispatches that hook for root screens, and this screen is pushed. KeyInfoScreen gains a with_parent breadcrumb crumb so the trail leads back to the screen the key was opened from. Parameterised, not hardcoded: it has nine parents and naming one of them for all of them would mislabel the other eight, which keep their two-level trail unchanged. Key role words now follow the identity that owns the key. The DIP-3 wording describes masternode registration duties — updating a registration, receiving node rewards — and on a user identity that does not read as jargon, it asserts the user owns a masternode. A user's TRANSFER key is a Transfer key, not a payout address. Both the keys list and the restore offer take the same vocabulary from the same identity: scoping only one would have a user's offer say "Payout address key" six lines above a list saying "Transfer key", which is the two-names-for-one-key defect the shared vocabulary exists to prevent. Empty-state copy no longer says "no keys saved on this device" — these rows are the identity's on-chain public keys, and that phrase means held=false per-row on this same screen. Track B, reads only, after verifying the read/write split against the source rather than the report: :746 (feeds the vault chokepoint, whose consequence is signing with and displaying the wrong key's material) and :887 (reload) are reads and now resolve the target the caller established. The keys list also resolves held-ness against both conventions, since real installs hold material written under each; looking in a second place can only find material that is already there. The writes are deliberately reverted to the purpose-derived target, which the previous commit had changed. QualifiedIdentity::sign and can_sign_with resolve that way, so material stored anywhere else is material that can never sign: for a voting key filed on the main identity, the previous commit would have taken a hand-entered private key and stored it where the signer never looks. Removal keeps the derived target for the same reason, which leaves wrong-key removal as a documented residual. Both sites carry the reasoning, and reconciling the two conventions is a migration this PR is not the place for. * fix(identity): one attribution rule for both hosts of the recovery offer The keys list gained typed error attribution while the Key Info screen kept ending a restore on any MessageType::Error, so the two screens hosting the same identity-scoped offer disagreed about whether that restore had ended. An unrelated task's failure still re-armed Restore on one of them. Attribution moves onto LegacyRecoveryState::owns_error, which already knows the identity it belongs to, and both screens call it from display_task_error. One implementation, so the two hosts cannot drift; returning false keeps AppState's banner, because the user still has to see the error. Key Info keeps its own blanket handling for the separate protection-migration gate, which is a different state machine with a different in-flight guard. * fix(identity): identify a key by its public half, and name it the same everywhere Two reviewers found the same hole in the two-candidate held-state lookup. It derived its fallback target by looking key_id up in the main identity's key set even while checking a voter-identity row, and KeyStorage::has only proves a slot is filled, not whose material fills it. A main identity holding a voting key at id N alongside a linked voting identity holding its own key at id N would report the main key as held on the strength of the other key's private half — and hand that material to Key Info. The loop already has the key, so the fallback comes from key.purpose(), and each candidate now has to hold this exact public key. Reverting to slot presence fails the new test. AC-6's third surface: the Key Info page still named its key by raw Debug purpose, so the list and the restore offer agreed with each other and not with the page they open. It now takes the same identity-scoped vocabulary, with the raw purpose kept as Expert detail rather than dropped. The CHANGELOG's known-limitation note said what the app does internally and left the reader nothing to do about it. It now names the situation, what can go wrong, and the check that resolves it — and drops the claim that saving uses "the older of the two places", which is not what makes saving risky. The collision with a same-numbered key is. * fix(identity): find a key's private half where it is filed, not where its purpose implies A key's private half lives in a BTreeMap keyed by (PrivateKeyTarget, KeyID), and the target was derived from the key's Purpose: every VOTING key to the voter identity, everything else to the main one. That derivation cannot express a voting-purpose key filed on the main identity, which is a supported shape -- masternode_key_presence reads it as voting readiness on its own, and load_identity, the authoritative loader, files a main-identity key under PrivateKeyOnMainIdentity whatever its purpose. So for that key sign and can_sign_with looked in a store it was never filed under: the app accepted the key, saved it, showed it as held, and no signing path could ever find it. Replaced by asking the store instead of the purpose. KeyStorage::candidates probes each target at the key's own id and keeps only entries whose stored public-key data matches the requested key -- three BTreeMap probes, not a scan. Matching on the id alone is what let a lookup land on a different key: the voter and main id spaces overlap, so id 0 names two keys on a masternode. resolve_private_key_bytes now takes the public key and discovers the placement, returning the first that actually YIELDS BYTES rather than the first that matches. A vault placeholder whose secret is gone can sit beside a live entry for the same key; stopping at the first match would report such a key unusable with its bytes one probe away. With nothing to fall through to, the dead placement still surfaces its own typed error, so "the vault is not open" never degrades into "you never had that key". Discovering the placement also fixes the vault label. The map key and the label identity_key_priv.<m|v|o>.<key_id> are one composite address, so a caller that passed a target could name a label the bytes were never stored under. With this signature it cannot pass one at all. This is placement, not derivation: an AtWalletDerivationPath entry can be correctly matched here and still carry a stale path, which is what the ECDSA_HASH160 recovery scan exists for. That scan is untouched. QualifiedIdentity::placement_of answers the other question -- where a private half SHOULD go -- from the identity's own on-chain lists, for the write path only. Unknown is a real state, not a default: a locally-added key is on no list until its state transition is broadcast. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(identity): Key Info names a user's keys the way the keys list does The restore offer is hosted by three screens — the keys list, Key Info, and masternode detail — and each names the keys it offers to bring back. Two of them were scoped to the identity's vocabulary; Key Info was not. A plain user's transfer key was therefore offered as a "Payout address key" there, asserting the user owns a masternode, and contradicting the keys list one screen away, which calls the same key a "Transfer key". Someone deciding whether to restore a key should not have to work out that the two names are the same key. The literal fix is one argument, but a third site forgetting the same call is a design problem, so the footgun goes with it: `vocabulary` moves from a builder option defaulting to the masternode wording to a required constructor argument. Omitting it is now a compile error rather than a plausible-looking wrong label, and a fourth host cannot inherit masternode words by accident. A required argument catches a *missing* vocabulary, not a *wrong* one, which is exactly what happened here — so the guard is at the host: the new test drives Key Info through the route the user takes, arms the offer, and reads the words off the rendered offer. Reinstating the wrong vocabulary fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(identity): a disabled key the device holds is still reported as held The stored copy of a public key is a snapshot taken when its private half was saved. A Platform identity public key is immutable once added with one exception — disabling rewrites `disabled_at` — so full-struct equality between the snapshot and the live key stopped matching the moment a key was disabled or rotated, and the row reported a key this device demonstrably holds as missing. Same symptom as the voter-identity lookup bug, different trigger. It fails safe, understating possession rather than over-, which is why it survived the earlier fix. `same_key` therefore excludes the one field that legitimately moves, and nothing else. Comparing just the id and the key material would also fix this and reopen a worse hole in the other direction: `id` is already the lookup key, and a main identity's voting key and a linked voter identity's key can carry identical `data`, leaving `purpose` as the only thing telling them apart. Conflating those reports the main identity's key as held on the strength of a different key's private half and hands that material to Key Info — the exact defect this lookup exists to prevent. Verified both ways: the narrow id+data comparison passes the new disabled-key test and fails `a_same_numbered_key_on_the_voter_identity_is_not_mistaken_for_this_one`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(identity): resolve a key's store on demand instead of threading it through screens KeyInfoScreen carried an Option<PrivateKeyTarget> that a caller could supply and fell back to the purpose derivation when none did. One caller of fifteen supplied it, and Screen::screen_type dropped it on every round trip, so even that one lost it. With the resolver in place the field has nothing left to do: the screen asks the identity where the key is each time it needs to know, so there is no state to thread and no caller left to forget it. target() now prefers an existing placement -- that is where the material is -- and falls back to the identity's own on-chain lists for where a new private half belongs. Both agree with where the resolver will look. It returns Option, because a key on none of this identity's lists with nothing held for it cannot be placed honestly; the write, display and sign paths say so in the user's words rather than guessing a store. The write reuses an existing placement, so re-entering a key overwrites it instead of growing a second copy under another store. The delete removes every placement holding that key, selected on key material, so a duplicate cannot survive the removal the user asked for and the removal cannot land on a different key that merely shares the id. keys_screen's filed_at probed the structural target then the purpose-derived one using KeyStorage::has, which matches on key id alone. That is the collision in read form: on a masternode the voter and main id spaces overlap, so a bare id hit could report an unheld key as held against another key's material. Replaced by the same resolver as everywhere else, which selects on material. With every reader migrated, impl From<Purpose> for PrivateKeyTarget is deleted. It compiled away with no fallout, which is the evidence that the derivation was fully contained -- and its absence is what stops a second derivation being reintroduced by accident. Also pins the real v0.9.3 blob's keys as reachable, not merely decodable. The existing test proves the bytes survive the bincode version gap; this one proves an upgraded masternode can still sign with its owner key and still vote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(masternodes): a key opened from a node's page keeps the name the list gave it The detail view's key rows already knew where each key's private half was — they looked there to decide whether it was held — but dropped that on the way to `KeyInfoScreen`, which then re-derived the location from the key's purpose. The two disagree for a key filed on the voter identity whose purpose is not `VOTING`, and `role_label_and_tip` exists precisely because that shape occurs: a key on a voter identity is the node's voting key whatever its purpose field says. So the list said "Voting key" and the page one click later said "Authentication key", about one key. The same wrong target drives the page's own re-read, so it also reported a key the device demonstrably holds as missing — the identity-path defect, on the masternode path. Forwarding the row's target closes both. The new test drives the real route (card → detail → key row → Key Info) rather than the pieces, since this drifted where nothing crossed the two surfaces; reverting the fix fails it on the name assertion. Also corrects a comment falsified by this branch: the identity keys list is no longer "the static read-only KeysScreen table". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(identity): make the key map private so the resolver cannot be bypassed With placement resolved from key material rather than derived, all of the correctness lives in the resolver. A caller reaching past it into the map does not merely look wrong -- it can miss a key that is present and report a saved key as absent. The field is now private, so reaching past it is a compile error instead of a review question. Direct field access is replaced by a named API that says which question it answers. candidates() is the one that finds a key. entry_at / insert_at / remove_at name a placement explicitly, for the callers that legitimately know one: a loader walking the identity list it read a key from, and legacy recovery, which is *about* the placements an old blob recorded and must not be routed through a target-blind resolver. iter / values / len / is_empty are target-blind walks. insert_if_absent replaces an `.entry().or_insert()` reach-through, and names what it is for: folding a previously-loaded record into a fresh one so a key the new load did not resupply is kept, without overwriting one it did. This is auditability, not a wall. A writer still has to choose a placement, so the point is that every such choice is now a named call a reviewer can find, rather than a map poke indistinguishable from a read. Two production readers were reaching in with a hardcoded main-identity target and are now resolved: get_selected_wallet in ui/identities, which finds the wallet a key derives from, and the withdrawal screen's wallet-unlock probe. Both would have missed a key filed anywhere else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(identity): record the key-placement resolution design and the fix The design note explains why placement is asked of the store rather than derived from the key's purpose, why resolution returns the first placement that yields bytes rather than the first that matches, and why the reconciliation migration this problem seemed to need was designed and then withdrawn -- with the resolver permanent, moving entries buys only one convention on disk, and the bytes it would move are irreplaceable. CHANGELOG describes it as the user meets it: a voting key that was saved, shown as present, and could not sign. No docs/kv-keys.md entry and no user-story change: the fix adds no durable state and restores behaviour already catalogued rather than introducing a feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(identity): one held-key resolution rule for every Manage keys surface `identity_keys()`'s doc comment claimed that sharing key enumeration is "what keeps the two surfaces agreeing on which keys are held". It wasn't true. Enumeration was shared; resolution was not. The identity keys list tried both filing conventions and verified the match, while the masternode detail view passed the structural target straight through — so a voting key held on the main identity but filed by purpose derivation, which is what entering one by hand produces, read as saved on the keys list and missing on the node's page. So `filed_at` and `same_key` move out of `keys_screen` to sit beside `identity_keys` as `key_filed_at`, and both surfaces call it. Enumerate alike, resolve alike. The doc comments on both now describe what the code does: the structural target is half of pairing a public key with its private material, and this is the other half. Only reads change. The two conventions still disagree on the write path; reconciling that is a migration, tracked separately. Coverage at both levels: a unit test next to the function pins all four properties of the rule (either convention found, wrong key at the same id rejected, `disabled_at` tolerated), and a kittest drives the node's real route to prove the surfaces now agree. Reverting `detail_screen` to the raw structural target fails the kittest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(identity): make an upstream key field a build failure, not a silent gap `same_key` compares an identity public key field by field, deliberately skipping `disabled_at`. Written as a chain of accessor calls, a field added upstream would simply not be compared: two keys differing only in that field would compare equal, and this function decides which stored private material belongs to which key. Destructuring `IdentityPublicKeyV0` exhaustively, without `..`, moves that decision to whoever adds the field — the build stops until they say whether it identifies a key or, like `disabled_at`, only describes its state. Verified the guard bites: dropping a field from the pattern fails with E0027 "pattern does not mention field", which is exactly what an upstream addition would produce. Behaviour is unchanged, so the existing tests hold as written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(identity): pin that removing one key spares a different key sharing its id The delete path had no test of its own -- the resolver tests covered which placements `candidates` returns, but nothing covered the removal that iterates them, and the resolver tests exercise new code so none of them could go red first. The removal moves into `remove_held_private_key` so it can be driven directly. The test builds the shape a real install reaches with two writers: the structural loader files a main-identity VOTING key under `Main`, while an older build's paste path filed an unrelated key under `Voter` at the same id -- ids overlap between the two spaces, so id 0 names two keys on a masternode. Confirmed RED against the retired purpose-derived removal, and the failure is the one that matters: it was the *first* assertion that broke, meaning the key the user asked to delete was still on the device afterwards, while the other key's private half had been removed instead. Design note updated: `same_key`'s reasoning in §2, and the three tests the merge added to §9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(masternodes): resolve the Add-protection CTA's held key like every other surface first_protectable_key still probed a raw structural (target, key_id) slot instead of going through candidates(), the one resolution rule the rest of this PR consolidated every other UI site onto. It could miss a key filed under a non-structural placement (e.g. a main-identity voting key an older build filed under the voter placement) and cloned raw key bytes out of the vault every frame doing it. Delegates to candidates().next().is_some() like its sibling open_key_info_with_mode a few lines down. Presence-only, so no secret clone. * fix(identity): reach Key Info's re-read, share the restore offer, name keys whole Three defects a review found on the Key Info navigation work. Key Info's arrival re-read hung off `refresh_on_arrival`, which `AppState` dispatches only to root screens. This screen is always pushed onto the screen stack, so the reload, the protection-status reset and the offer re-arm were unreachable: a write landing off-screen was erased by the next ordinary key edit, which persists the whole identity clone the screen opened with. The body moves onto `refresh`, which does reach a pushed screen; `refresh_on_arrival` delegates there by default, so both hooks still run it. The regression test moves to kittest and drives the app's own `TaskResult::Refresh` dispatch, since hand-calling the hook passed against the bug. The offer to restore stranded keys was hand-copied across its three hosts: render, result routing, error routing, dispatch. The render chain becomes `legacy_recovery_section::host_offer` and the attribution rules become `LegacyRecoveryState::absorb_result` / `absorb_error`, leaving each screen only its own side-effects. A shared rule applied to two hosts of three is what caused an earlier bug here; a fourth host now cannot repeat it. Key role labels were assembled from fragments — a bare role word, an English noun, a state, a positional key id — and two surfaces interpolated a raw `Purpose` through `Debug`. `role_label_and_tip` now returns whole `&'static str` captions keyed on vocabulary, purpose and retired state, so composing one at a callsite is impossible rather than discouraged, and matches every `Purpose` by name so a variant added upstream breaks this build instead of reaching a user as `{:?}`. The raw purpose keeps its place on Key Info as its own Expert field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(user-stories): point IDN-008 at the key list that shipped The catalog entry for viewing identity keys still described criteria that predate a reachable route into a key's own page — this branch is what first gives `KeysScreen` a way into `KeyInfoScreen` at all. The real criteria had landed under IDN-020, a legacy-migration story, and IDN-008 also overstated the Everyday view: the new list gates key id, purpose, security level, type and read-only behind the Expert view. Restates the criteria as what ships — reachable from the identity's Settings → Advanced without changing interface mode, every key openable whether or not the device holds it, role and held state in words, on-chain specifics as Expert detail — and cross-references IDN-020 for the restore offer rather than duplicating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(identity): route legacy-recovery test fixtures through insert_at/has The merge of fix/889-key-info-navigation brought in test fixtures written against KeyStorage's pre-privatization field (private_keys.private_keys), which this branch made private so callers cannot bypass the resolver (design.md §7). No textual conflict flagged it since the two branches touched different files; cargo build --all-features --tests caught it. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 62b804bc2da189f14c715bda84816732ff56986c)
orchardpaytl
pushed a commit
to orchardpaytl/orchardpay
that referenced
this pull request
Jul 30, 2026
…ge API changes Commit dashpay#946 changed resolve_private_key_bytes to take a &IdentityPublicKey instead of a (PrivateKeyTarget, KeyID) pair, and made KeyStorage's private_keys field private. Update the one OrchardPay-only caller and two test fixtures that constructed KeyStorage directly to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: Fixes a key-storage inconsistency where a private key's saved location was decided two different ways in different parts of the app, which could make a held key unusable for signing, or make deleting one key accidentally delete a different key's private data.
User story
As an Everyday User or masternode operator, I want the app to reliably find and use a private key I've entered or restored regardless of which internal convention filed it, to achieve confidence that a key that shows as "held" actually works when I need to sign or vote with it, and that deleting a key never touches a different key by mistake.
Scenario
Base flow
A private key's saved location is recorded as a (placement, key-id) pair — placement being the identity it "belongs" to (the main identity, a linked voter identity, or a linked operator identity). Two independent parts of the app historically decided that placement differently: one from the on-chain structure (which identity's key list a key actually sits in), the other purely from the key's stated purpose (assuming, for example, that any voting-purpose key belongs to a voter identity).
Actual behavior
Those two derivations agree in the common case but disagree whenever a voting-purpose key is held directly on the main identity (a real, supported shape) — or, symmetrically, when a non-voting key is held on a linked voter identity. In that shape:
Expected behavior
The app finds and uses a key's private half by matching stored public-key material against the key actually in front of it, never by assuming where the key ought to be based on its purpose. A key is found wherever it's actually filed; a delete only ever removes the specific key it was asked to remove.
Detailed discussion
What was done
impl From<Purpose> for PrivateKeyTarget, deleted) with a resolver that locates a key's stored private half by trying every placement a key can legitimately have and accepting a candidate only when its stored public-key material matches the key being looked up (id, purpose, security level, contract bounds, key type, and data — deliberately excludingdisabled_at, since a key can legitimately be disabled on-chain after its private half was saved).sign/can_sign_withand the vault-facing read/write/delete paths inKeyInfoScreennow go through this resolver instead of re-deriving a placement from purpose;resolve_private_key_bytestakes the public key itself rather than a caller-supplied placement, so no call site can hand it a mismatched target.purposeas a discriminator, not just id and public-key data — was kept.Testing
Unit tests cover: a purpose-derivation-vs-actual-placement mismatch on both the main and a linked voter identity (the exact previously-broken sign/can-sign case); a same-numbered-key collision on delete (confirmed removing only the intended key's material, by first reproducing the bug against the old purpose-derived delete and then against the fix); fallthrough when one of two matching placements has no retrievable private bytes; probe-order determinism; the immutability assumption the resolver depends on; and the real v0.9.3 legacy-format fixture, confirming both keys it carries (owner and voting) are reachable, not merely decodable.
cargo test --all-features --lib --test kittest(2220 lib + 312 UI tests) andcargo clippy --all-features --all-targets -- -D warningsclean;cargo fmt --allapplied.Breaking changes
None. No schema change, no new durable state, no change to any already-working flow — this restores correct behavior for a shape that was already possible before this fix, not a new capability.
Checklist
cargo fmt --allcargo clippy --all-features --all-targets -- -D warningsCHANGELOG.mdupdateddocs/ai-design/2026-07-30-key-placement-resolution/)Prior work
Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent