fix(identity): close two rounds of key-placement-resolution review findings - #948
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>
…failure resolve_private_key_bytes kept the first failure it saw and only broke the walk on a cancellation, so a dead placeholder probed ahead of the sealed placement the user declined to unlock reported its own mechanical failure — the user dismissed a password prompt and was told the key is missing from this device. The cancellation now returns as-is: the user's decision is the answer about the key, whatever an earlier store failed with. Co-Authored-By: Claude fable <noreply@anthropic.com>
The paste path still marked the key held one statement before the write that could refuse it: on a password-protected identity the keyless persist is refused every time, yet the screen kept offering to sign with, reveal and remove a key the record never accepted — and blamed disk space for a refusal whose typed message names the actual remedy. The held-state assignment now follows a successful persist, the in-memory key map rolls back on refusal, and both the paste and remove refusals speak through the typed error's own message. The removal path gets the same rollback: a persist refusal no longer leaves the screen claiming a key is gone that the record on disk still holds. Beyond the reviewed lines, applied for consistency — it is the same divergence in the same function family. Co-Authored-By: Claude fable <noreply@anthropic.com>
The display/sign path collapsed both placement failures into one hardcoded sentence advising the user to enter the private key on this page — for a key on two lists at once, the exact action the same ambiguity then refuses, with the real remedy sitting in the discarded typed error. The site now surfaces the typed message like the paste path's sibling refusals already do. Co-Authored-By: Claude fable <noreply@anthropic.com>
…ed key The popup still probed the key store by bare (target, id) — the derivation pattern this series removes everywhere else — so a key an older build filed under the other identity's store rendered as unsaved and opened its Key Info page in the wrong state. Both key loops now ask held_private_key_data, the same placement-blind rule every other synchronous lookup shares. Co-Authored-By: Claude fable <noreply@anthropic.com>
The refusal told the user to refresh the identity, but a refresh updates published keys and evicts no locally saved private half — and the backend add path has refetched the identity moments before the refusal fires, so following the advice recomputes the identical collision with no exit. The message now names the performable remedy: open the occupying key and remove its saved private key from this device. The add-key comment claiming the minted slot is free by construction is corrected too — max_id is the published record's, the slot check is against the local store, and the two can disagree. Co-Authored-By: Claude fable <noreply@anthropic.com>
…robe budget first_live_candidate and wallet_derived_at collected the lazy candidates iterator into a Vec each call — two heap allocations per key per frame on the keys list, for an iterator documented as three map probes. Both now chain the iterator directly; the liveness fallback re-probes at the cost of at most three more map reads. The get_selected_wallet test also loses its seventeen-field QualifiedIdentity literal to a local fixture helper, so it shows only the three things it varies. Co-Authored-By: Claude fable <noreply@anthropic.com>
design.md caught up with the branch it describes: §2 gains the slot guard's real precondition and first_live_candidate, the synchronous approximation every screen shares; §3 states the cancellation carve-out; §8 records the grovestark unpublished-key deferral next to its source TODO; §9's coverage table gains a layer column — telling the near-homonym resolver/naming/store tests apart — and the rows this branch added. The two accepted residuals are marked in place: a TODO on KeyStorage's overstated pub-surface narrowing (mark_in_vault et al. still take a caller-named placement), and an INTENTIONAL marker on the bincode unmaintained-advisory pin whose rationale Cargo.toml already carried. CHANGELOG entries are reworded to stay truthful to the fixed behavior. Co-Authored-By: Claude fable <noreply@anthropic.com>
…tests Co-Authored-By: Claude fable <noreply@anthropic.com>
The agent's final polish commit (905e262) reported a clean fmt check, but coordinator-side re-verification found one unformatted call still present. cargo fmt --all fixes it; no logic change. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…nsistency' into fix/889-key-resolution-triage-followup # Conflicts: # src/ui/masternodes/detail_screen.rs
…ution-triage-followup # Conflicts: # CHANGELOG.md # docs/ai-design/2026-07-30-key-placement-resolution/design.md # src/backend_task/grovestark.rs # src/model/qualified_identity/encrypted_key_storage.rs # src/model/qualified_identity/key_placement.rs # src/model/qualified_identity/mod.rs # src/ui/identities/keys/key_info_screen.rs # src/ui/identities/keys/keys_screen.rs # src/ui/identities/mod.rs # src/ui/identities/withdraw_screen.rs # src/ui/masternodes/detail_screen.rs # tests/kittest/key_info_screen.rs
Contributor
📝 WalkthroughWalkthroughThe PR updates key placement resolution and storage safeguards, validates vault key material, adds atomic identity-key removal, and exposes locally held unpublished keys in the UI. It also updates wallet association, key-opening flows, tests, documentation, and changelog entries. ChangesKey placement and lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant KeysScreen
participant KeyStorage
participant KeyInfoScreen
participant IdentityDB
participant Vault
User->>KeysScreen: open unpublished held key
KeysScreen->>KeyStorage: retrieve held private-key data
KeysScreen->>KeyInfoScreen: open key details
KeyInfoScreen->>IdentityDB: resolve and persist key operation
KeyInfoScreen->>Vault: validate or remove vault secret
Vault-->>KeyInfoScreen: result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…le' into fix/889-key-resolution-triage-followup # Conflicts: # docs/ai-design/2026-07-30-key-placement-resolution/design.md # src/ui/identities/keys/key_info_screen.rs
4 tasks
…rompts resolve_private_key_bytes walked candidates in bare probe order, so a Tier-2-sealed copy of a key probed ahead of a sibling placement holding the same bytes in the clear would open a password prompt for material the identity already holds — and a cancelled prompt then denied the key outright, contradicting first_live_candidate's resident-first rule from the same design. The walk now takes prompt-free placements first, which also makes the cancellation short-circuit provably correct: it can only fire when no resident copy existed to serve. Co-Authored-By: Claude fable <noreply@anthropic.com>
…ult placeholder with_identity_secret_key fetched the vault strictly at the placement the caller named. Key Info names that placement through the synchronous approximation, which cannot see whether a vault label is live — so a key filed InVault under two stores with only one live secret (a blob restored without its vault, an interrupted seal) had Show and Sign fail on the dead placeholder while the bytes sat one probe away, the exact shadowing the honest resolver's fallthrough rule exists to prevent. The chokepoint now probes the named placement and the key's sibling placements for a live label and serves the first, keeping the recorded-key mismatch check on whatever placement actually answers. Co-Authored-By: Claude fable <noreply@anthropic.com>
… the write path IdentityKeyPlacementAmbiguous and IdentityKeyNotOnIdentityRecord got their remedies written around the paste path, but filed_at() serves the Show/Sign dispatch too — so a user who pressed Show, having entered nothing and asked to save nothing, was told to try saving the key again. Both messages now name the one step either path can perform: refresh the identity and open the key again. Co-Authored-By: Claude fable <noreply@anthropic.com>
… fallthrough Both landed user-visible in the two preceding fixes; the release note block covering this key-resolution work should carry them. Co-Authored-By: Claude fable <noreply@anthropic.com>
…of the record Both paths persisted the whole identity clone taken when Key Info opened, guarded only by the write-side lock — so a key another writer landed while the screen was open (a finished AddKeyToIdentity, a legacy restore, a second screen) was silently written away by the next paste or removal, possibly erasing a private half's only copy. Each edit now runs through AppContext::edit_local_qualified_identity, which re-reads the record under identity_record_lock, applies the edit to what is on disk now, persists through the existing locked write half, and returns the record for the screen to adopt. The placement decision and occupied-slot check also move onto the fresh record, and the snapshot-rollback idiom disappears — the screen no longer mutates its clone speculatively, which also stops those unwiped KeyStorage snapshots being cloned on the two paths handling raw key bytes. Co-Authored-By: Claude fable <noreply@anthropic.com>
…oolean Both loops of the identities list's Keys popup called held_private_key_data per key per frame just to pick a fill colour — copying raw key bytes for every plaintext-carrying key on every render, the exact use its own rustdoc forbids. The popup now answers "is this key held" with the candidates presence probe, the way the keys list and the masternode detail screen already do, and fetches the material only on the click that hands it to Key Info. Co-Authored-By: Claude fable <noreply@anthropic.com>
The occupied-slot refusal tells the user to open the conflicting key in this identity's key list and remove its saved private half — but the occupant add_key_to_identity refuses on is a locally saved, unpublished entry, and every key-list surface enumerates published keys only, so from that caller the remedy named a row that did not exist. The keys list now shows held-but-unpublished keys in their own section, hidden when there are none, each row opening Key Info where the removal already works — making the refusal's remedy performable from both of its callers without weakening the message. Co-Authored-By: Claude fable <noreply@anthropic.com>
… the code enforces design.md §7 asserted that what remains pub on KeyStorage cannot file a key at a placement of the caller's choosing, while the struct's own TODO(placement-named-pub-surface) calls that overstated — mark_in_vault is pub, caller-named and zeroizes the occupant with no same_key guard. §7 now states the write-side narrowing that actually holds and names the residual pub surface, §8 carries the matching residual bullet so the two markers point at each other the way the grovestark pair does, and the code TODO points back at both. Co-Authored-By: Claude fable <noreply@anthropic.com>
… the fix set clippy's argument-count lint was right that key_row took its label and tooltip as two loose halves of what manage_keys_labels yields as one entry; pass the pair whole. Also records the two §9 rows the audit table was missing for this round's message and keys-list tests. Co-Authored-By: Claude fable <noreply@anthropic.com>
…gh chokepoint The dispatch comment still described the named placement as the one place the material can be, and a test assertion still spoke of a rollback the locked read-modify-write no longer performs. Co-Authored-By: Claude fable <noreply@anthropic.com>
Claudius-Maginificent
marked this pull request as ready for review
July 30, 2026 23:49
Collaborator
|
⛔ Blockers found — Sonnet deferred (commit fcdde0f) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/model/qualified_identity/encrypted_key_storage.rs (1)
529-538: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe synchronous "resident" rule and §3's async rule disagree about wallet-derived placements.
first_live_candidateexcludes onlyInVault, whileresolve_private_key_bytestreats bothInVaultand wallet-derived placements as prompting — so for a dual-filed key the screen can name anAtWalletDerivationPathplacement (needing a wallet unlock) while the resolver would serve a prompt-freeClearsibling.
src/model/qualified_identity/encrypted_key_storage.rs#L529-L538: also skip placements wherewallet_seed_hash_for(placement).is_some()in the first pass, keeping the existing.or_else(first candidate)fallback.docs/ai-design/2026-07-30-key-placement-resolution/design.md#L67-L74: state precisely which placements the synchronous approximation treats as resident, so §2 and §3 describe the same rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/model/qualified_identity/encrypted_key_storage.rs` around lines 529 - 538, Update first_live_candidate in src/model/qualified_identity/encrypted_key_storage.rs#L529-L538 so its first pass skips both InVault placements and placements where wallet_seed_hash_for(placement).is_some(), while preserving the existing first-candidate fallback; update docs/ai-design/2026-07-30-key-placement-resolution/design.md#L67-L74 to precisely define these resident placements consistently across §2 and §3.src/backend_task/grovestark.rs (1)
44-49: 📐 Maintainability & Code Quality | 🔵 TrivialTODO has no tracking issue.
Every other residual in
design.md§8 is at least named; this one only lives in a code comment. Want me to open an issue for the unpublished-key proof-generation gap so it isn't lost?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend_task/grovestark.rs` around lines 44 - 49, The TODO in the grovestark key-resolution path lacks a trackable issue reference. Create or identify a tracking issue for the unpublished-key proof-generation gap, then update the TODO comment near resolve_private_key_bytes to include that issue identifier while preserving its existing context.src/backend_task/wallet/mod.rs (1)
583-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo case pins the "unverifiable is not wrong" skip branch.
Lines 190-193 deliberately let a key type whose public half this build cannot derive through unchecked. Nothing here asserts that, so a future tightening of the
if let Ok(..)into a hard failure would pass the suite while breaking BLS/EdDSA identity keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend_task/wallet/mod.rs` around lines 583 - 742, Add a regression test covering the unverifiable-key branch in with_identity_secret_key: use an identity key type whose public key cannot be derived by this build, place the matching secret in the vault, and assert the closure still executes successfully with the secret bytes. Anchor the test beside a_secret_matching_its_recorded_key_still_resolves and preserve the behavior that failure to derive the public half is treated as unverifiable, not as IdentityKeyMismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 156-172: Update the “One known limitation” paragraph in
CHANGELOG.md to remove or rewrite the outdated warning about same-numbered
voting keys and the in-progress key-placement fix. Ensure the release notes no
longer describe the behavior addressed by the entries above as an unresolved
limitation.
---
Nitpick comments:
In `@src/backend_task/grovestark.rs`:
- Around line 44-49: The TODO in the grovestark key-resolution path lacks a
trackable issue reference. Create or identify a tracking issue for the
unpublished-key proof-generation gap, then update the TODO comment near
resolve_private_key_bytes to include that issue identifier while preserving its
existing context.
In `@src/backend_task/wallet/mod.rs`:
- Around line 583-742: Add a regression test covering the unverifiable-key
branch in with_identity_secret_key: use an identity key type whose public key
cannot be derived by this build, place the matching secret in the vault, and
assert the closure still executes successfully with the secret bytes. Anchor the
test beside a_secret_matching_its_recorded_key_still_resolves and preserve the
behavior that failure to derive the public half is treated as unverifiable, not
as IdentityKeyMismatch.
In `@src/model/qualified_identity/encrypted_key_storage.rs`:
- Around line 529-538: Update first_live_candidate in
src/model/qualified_identity/encrypted_key_storage.rs#L529-L538 so its first
pass skips both InVault placements and placements where
wallet_seed_hash_for(placement).is_some(), while preserving the existing
first-candidate fallback; update
docs/ai-design/2026-07-30-key-placement-resolution/design.md#L67-L74 to
precisely define these resident placements consistently across §2 and §3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 366082ba-d0e0-4afa-bdb7-5c5377ebefa3
📒 Files selected for processing (24)
CHANGELOG.mdCargo.tomldocs/ai-design/2026-07-30-key-placement-resolution/design.mddocs/user-stories.mdsrc/backend_task/error.rssrc/backend_task/grovestark.rssrc/backend_task/identity/add_key_to_identity.rssrc/backend_task/wallet/mod.rssrc/context/identity_db.rssrc/model/qualified_identity/encrypted_key_storage.rssrc/model/qualified_identity/key_placement.rssrc/model/qualified_identity/mod.rssrc/ui/components/message_banner.rssrc/ui/identities/identities_screen.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/identities/keys/keys_screen.rssrc/ui/identities/mod.rssrc/ui/identities/withdraw_screen.rssrc/ui/masternodes/detail_screen.rstests/backend-e2e/identity_in_vault_sign.rstests/backend-e2e/z_broadcast_st_tasks.rstests/kittest/identities_screen.rstests/kittest/key_info_screen.rstests/kittest/keys_screen.rs
Comment on lines
+156
to
+172
| - **A key two lists appear to share can be saved again**: when a masternode's | ||
| own record and its voting identity each carried a key with the same number and | ||
| the same public key, entering the private key of either was refused with a | ||
| message saying the key does not belong to this identity — although it plainly | ||
| does, and what the two keys are for is what tells them apart. Such a key is | ||
| now saved where it belongs. When a key really is on two lists at once, the | ||
| message now says so and what to do about it. | ||
|
|
||
| - **Entering a key can no longer erase a different one**: keys of a masternode's | ||
| own record and of its voting identity are numbered separately, so two | ||
| different keys can carry the same number. Entering the private key of one of | ||
| them used to take the other's place without a word, and the replaced key's | ||
| private half was gone — with no copy to restore it from if it had been | ||
| imported by hand. Dash Evo Tool now refuses that and explains what happened, | ||
| leaving the saved key untouched. Re-entering a key you already saved still | ||
| replaces itself, as before. | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale caveat elsewhere in the same release now contradicts these entries.
The unchanged "One known limitation" paragraph (around lines 225-233) still tells users that saving or removing a voting key can affect a same-numbered key on a linked voting identity, and points at an "in-progress key-placement resolution fix" — which is this change. Please drop or rewrite that paragraph so the release notes don't warn about the behaviour they also announce as fixed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 156 - 172, Update the “One known limitation”
paragraph in CHANGELOG.md to remove or rewrite the outdated warning about
same-numbered voting keys and the in-progress key-placement fix. Ensure the
release notes no longer describe the behavior addressed by the entries above as
an unresolved limitation.
thepastaclaw
suggested changes
Jul 31, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The PR substantially improves placement-aware key resolution, persistence, vault verification, and recovery, but three in-scope correctness issues remain. Synchronous and wallet selection can still bypass a usable resident copy, while multi-placement vault deletion can partially remove a key despite reporting failure. There are also smaller issues in unsupported-key validation, unpublished-key naming, and the release notes.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 3 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/model/qualified_identity/encrypted_key_storage.rs`:
- [BLOCKING] src/model/qualified_identity/encrypted_key_storage.rs:535-537: Synchronous resolution can prefer a prompting or unusable placement
`first_live_candidate` treats every non-vault placement as resident, including `AtWalletDerivationPath`, which may require unlocking a wallet, and legacy `Encrypted`, which this code cannot resolve. If either variant is filed under an earlier target and the same key has a `Clear` or `AlwaysClear` copy under a later target, `held_private_key_data` makes Key Info prompt for the wallet or report the key unavailable. In contrast, `resolve_private_key_bytes` continues to the prompt-free plaintext copy. This breaks the PR's resident-first invariant and makes the synchronous UI disagree with the authoritative resolver.
- [BLOCKING] src/model/qualified_identity/encrypted_key_storage.rs:560-565: Do not require a wallet when a resident copy is usable
`wallet_derived_at` returns a path from any duplicate placement even when another placement contains immediately usable plaintext. Callers treat a returned path as proof that the operation needs that wallet: `WithdrawalScreen`, for example, returns before rendering the form while the wallet is locked. The actual key resolver uses the resident copy without touching the wallet, so a later wallet-derived duplicate can unnecessarily block an operation. The new `a_wallet_is_found_under_a_later_placement_too` test pins exactly this clear-first, wallet-later behavior and should instead verify that later wallet lookup is used only when no resident copy can answer.
In `src/context/identity_db.rs`:
- [BLOCKING] src/context/identity_db.rs:1181-1182: Multi-placement vault deletion is not failure-atomic
The new per-key deletion delegates to `IdentityKeyView::delete_all`, which performs one vault write per label, continues after an error, and returns only the first error afterward. For a key filed under multiple placements, one deletion can therefore succeed while another fails. `remove_held_private_key` then aborts before removing or persisting the map entries, leaving the record and UI claiming the key is fully held even though one or more vault copies are gone. This contradicts the documented and changelog guarantee that a vault failure leaves the key exactly unchanged. The labels must be deleted through a failure-atomic batch operation or restored on failure; merely stopping at the first error would still permit partial deletion when a later write fails.
In `src/backend_task/wallet/mod.rs`:
- [SUGGESTION] src/backend_task/wallet/mod.rs:190-200: Reject vault keys whose correspondence cannot be verified
The `if let Ok(derived)` check treats every derivation failure as permission to use the vault bytes unchecked. With the current DPP implementation, `BIP13_SCRIPT_HASH` always returns `NotSupported`, so `DeriveIdentityKeyForDisplay` can present any valid secp256k1 scalar stored at that label as the private key for the script-hash record even though no correspondence was established. The signing task rejects this key type, but the display/export path does not. Fail closed with a typed unverifiable-key error when derivation is unsupported instead of handing unverified bytes to the closure.
In `src/ui/identities/keys/keys_screen.rs`:
- [SUGGESTION] src/ui/identities/keys/keys_screen.rs:170-178: Use the same role fallback for unpublished rows and Key Info
An unpublished row is named from the historical storage target, while `KeyInfoScreen::published_on` defaults an unknown placement to `PrivateKeyOnMainIdentity`. An unpublished authentication key stored under `PrivateKeyOnVoterIdentity` is consequently called “Voting key” in the list and “Authentication key” after it is opened. Because no on-chain list publishes this key, its storage address is not a published role. Use the same main-identity fallback as Key Info so the shared naming rule remains consistent.
In `CHANGELOG.md`:
- [SUGGESTION] CHANGELOG.md:225-233: Remove the stale key-placement limitation
This paragraph still warns that saving or removing a same-numbered voting key can affect the linked identity's key, may leave the requested key in place, and will be fixed by an in-progress key-placement change. Those are the behaviors the preceding entries and this PR claim to resolve. Remove the paragraph or replace it only with limitations that remain after this change so the release notes do not describe the current fix as unfinished.
Comment on lines
+535
to
+537
| self.candidates(key) | ||
| .find(|placement| !self.is_in_vault(placement)) | ||
| .or_else(|| self.candidates(key).next()) |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Synchronous resolution can prefer a prompting or unusable placement
first_live_candidate treats every non-vault placement as resident, including AtWalletDerivationPath, which may require unlocking a wallet, and legacy Encrypted, which this code cannot resolve. If either variant is filed under an earlier target and the same key has a Clear or AlwaysClear copy under a later target, held_private_key_data makes Key Info prompt for the wallet or report the key unavailable. In contrast, resolve_private_key_bytes continues to the prompt-free plaintext copy. This breaks the PR's resident-first invariant and makes the synchronous UI disagree with the authoritative resolver.
Suggested change
| self.candidates(key) | |
| .find(|placement| !self.is_in_vault(placement)) | |
| .or_else(|| self.candidates(key).next()) | |
| self.candidates(key) | |
| .find(|placement| { | |
| matches!( | |
| self.private_keys.get(placement), | |
| Some(( | |
| _, | |
| PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_) | |
| )) | |
| ) | |
| }) | |
| .or_else(|| self.candidates(key).next()) |
source: ['codex']
Comment on lines
+560
to
+565
| pub fn wallet_derived_at(&self, key: &IdentityPublicKey) -> Option<&WalletDerivationPath> { | ||
| self.candidates(key) | ||
| .find_map(|placement| match self.private_keys.get(&placement) { | ||
| Some((_, PrivateKeyData::AtWalletDerivationPath(path))) => Some(path), | ||
| _ => None, | ||
| }) |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Do not require a wallet when a resident copy is usable
wallet_derived_at returns a path from any duplicate placement even when another placement contains immediately usable plaintext. Callers treat a returned path as proof that the operation needs that wallet: WithdrawalScreen, for example, returns before rendering the form while the wallet is locked. The actual key resolver uses the resident copy without touching the wallet, so a later wallet-derived duplicate can unnecessarily block an operation. The new a_wallet_is_found_under_a_later_placement_too test pins exactly this clear-first, wallet-later behavior and should instead verify that later wallet lookup is used only when no resident copy can answer.
Suggested change
| pub fn wallet_derived_at(&self, key: &IdentityPublicKey) -> Option<&WalletDerivationPath> { | |
| self.candidates(key) | |
| .find_map(|placement| match self.private_keys.get(&placement) { | |
| Some((_, PrivateKeyData::AtWalletDerivationPath(path))) => Some(path), | |
| _ => None, | |
| }) | |
| if self.candidates(key).any(|placement| { | |
| matches!( | |
| self.private_keys.get(&placement), | |
| Some(( | |
| _, | |
| PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_) | |
| )) | |
| ) | |
| }) { | |
| return None; | |
| } | |
| self.candidates(key) | |
| .find_map(|placement| match self.private_keys.get(&placement) { | |
| Some((_, PrivateKeyData::AtWalletDerivationPath(path))) => Some(path), | |
| _ => None, | |
| }) |
source: ['codex']
Comment on lines
+1181
to
+1182
| crate::wallet_backend::IdentityKeyView::new(&self.secret_store, identity_id.to_buffer()) | ||
| .delete_all(placements) |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Multi-placement vault deletion is not failure-atomic
The new per-key deletion delegates to IdentityKeyView::delete_all, which performs one vault write per label, continues after an error, and returns only the first error afterward. For a key filed under multiple placements, one deletion can therefore succeed while another fails. remove_held_private_key then aborts before removing or persisting the map entries, leaving the record and UI claiming the key is fully held even though one or more vault copies are gone. This contradicts the documented and changelog guarantee that a vault failure leaves the key exactly unchanged. The labels must be deleted through a failure-atomic batch operation or restored on failure; merely stopping at the first error would still permit partial deletion when a later write fails.
source: ['codex']
Comment on lines
+190
to
+200
| if let Ok(derived) = recorded | ||
| .key_type() | ||
| .public_key_data_from_private_key_data(key, network) | ||
| && derived.as_slice() != recorded.data().as_slice() | ||
| { | ||
| tracing::warn!( | ||
| identity = %identity_id, | ||
| key_id, | ||
| "Vault key at the requested placement is not the key recorded there", | ||
| ); | ||
| return Err(TaskError::IdentityKeyMismatch); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Reject vault keys whose correspondence cannot be verified
The if let Ok(derived) check treats every derivation failure as permission to use the vault bytes unchecked. With the current DPP implementation, BIP13_SCRIPT_HASH always returns NotSupported, so DeriveIdentityKeyForDisplay can present any valid secp256k1 scalar stored at that label as the private key for the script-hash record even though no correspondence was established. The signing task rejects this key type, but the display/export path does not. Fail closed with a typed unverifiable-key error when derivation is unsupported instead of handing unverified bytes to the closure.
source: ['codex']
Comment on lines
+170
to
+178
| for ((target, _), (stored, _)) in self.identity.private_keys.iter() { | ||
| let key = &stored.identity_public_key; | ||
| if !matches!(self.identity.placement_of(key), KeyPlacement::Unknown) { | ||
| continue; | ||
| } | ||
| if keys.iter().any(|(_, listed)| same_key(listed, key)) { | ||
| continue; | ||
| } | ||
| keys.push((target.clone(), key.clone())); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Use the same role fallback for unpublished rows and Key Info
An unpublished row is named from the historical storage target, while KeyInfoScreen::published_on defaults an unknown placement to PrivateKeyOnMainIdentity. An unpublished authentication key stored under PrivateKeyOnVoterIdentity is consequently called “Voting key” in the list and “Authentication key” after it is opened. Because no on-chain list publishes this key, its storage address is not a published role. Use the same main-identity fallback as Key Info so the shared naming rule remains consistent.
Suggested change
| for ((target, _), (stored, _)) in self.identity.private_keys.iter() { | |
| let key = &stored.identity_public_key; | |
| if !matches!(self.identity.placement_of(key), KeyPlacement::Unknown) { | |
| continue; | |
| } | |
| if keys.iter().any(|(_, listed)| same_key(listed, key)) { | |
| continue; | |
| } | |
| keys.push((target.clone(), key.clone())); | |
| for (_, (stored, _)) in self.identity.private_keys.iter() { | |
| let key = &stored.identity_public_key; | |
| if !matches!(self.identity.placement_of(key), KeyPlacement::Unknown) { | |
| continue; | |
| } | |
| if keys.iter().any(|(_, listed)| same_key(listed, key)) { | |
| continue; | |
| } | |
| keys.push(( | |
| PrivateKeyTarget::PrivateKeyOnMainIdentity, | |
| key.clone(), | |
| )); | |
| } |
source: ['codex']
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
Follow-up to #946 (merged): closes 14 findings from that PR's first code review, 9 more from a second review of this branch itself, 2 fixes absorbed from #947 (now closed, merged in here instead of staying a separate PR) for a removed key's vault secret being left behind and an unverified vault-to-record match on Show/Sign, and — as of this update — the final 7 blocking findings from a combined review of the whole branch. No new user-facing feature beyond one small addition: a new section in the Keys screen listing held keys not published on any on-chain key list, so users have somewhere to find and manage a key that would otherwise be invisible. Otherwise this fixes edge cases in the key-placement-resolution redesign: better error messages when a key can't be saved or removed, a fix so cancelling a password prompt doesn't wrongly block a key that didn't need one, a removed key's saved copy actually being deleted everywhere, key paste/removal now a properly locked read-modify-write instead of a snapshot-and-rollback that could lose concurrent writes, and several places where the app now finds a key's data correctly instead of missing it under certain conditions.
User story
As a user managing identity keys (pasting a private key, removing one, or viewing/signing with one), I want the app to correctly find, save, and report on my keys in every configuration they might legitimately be stored in, to have "remove" actually remove everything, to get an error message that actually tells me what to do when something goes wrong, and to be able to find a key I hold even if it isn't published anywhere on-chain yet.
Scenario
Actual behavior (before this branch, on top of #946)
Several review-found edge cases: a persist failure could leave the screen claiming a key was saved when it wasn't; a concurrent write to the identity record while the Key Info screen was open could be silently lost; a cancelled password prompt could block a sibling key placement that needed no password at all, or block Show/Sign from reaching a live copy of a key when the placement the app checked first was a dead vault placeholder; an "already occupied" error told the user to do something that wasn't actually possible; the identities list's Keys popup could miss a key filed under an older layout convention; removing a key's saved copy left its secret behind in the vault forever, unreachable even by deleting the identity; nothing verified a vault-stored secret actually matched the key it was being shown or signed as; a key held locally but not published on any on-chain key list had nowhere in the UI to be found.
Expected behavior (after this branch)
Each of the above is fixed with a pinned regression test. See
docs/ai-design/2026-07-30-key-placement-resolution/design.mdon this branch for the full design and its "what is not covered" section for what's still knowingly deferred.Detailed discussion
Stacked on: #946 (merged into
v1.0-dev). This branch carries #946's full history plusv1.0-dev's current tip, merged in twice (first the pre-squash branch tip, then the actual squash) to avoid resurrecting an already-fixed bug from a stale squash — both independently verified.Absorbed, not stacked: #947 (
fix/889-key-vault-secret-lifecycle) was a sibling branch fixing 2 issues this branch's own review also flagged. Rather than merge two overlapping PRs intov1.0-devseparately (both modified the same function), #947's 2 commits were merged directly into this branch, the one real conflict resolved by composing both invariants (vault-delete-first ordering from #947, persist-failure rollback from this branch) rather than picking one — verified via the full test suite that both hold together. #947 is now closed.Review status: this branch has been through 4 review passes, all closed:
AppContext::edit_local_qualified_identity), Show/Sign now fall through a dead vault placeholder to a live sibling instead of failing, the resolver serves prompt-free resident keys before ever prompting, the placement error messages are reworded to fit both the read and write path, the Keys popup no longer clones key material just to decide a tint, the design doc's claimed public-surface narrowing is corrected to what the code actually enforces, and the new "held but unpublished" Keys-screen section (feature addition, called out separately from the bug fixes above) gives both callers of the reworded "occupied slot" error a real place to point users at.Of pass 3's remaining 10 non-blocking / out-of-scope-follow-up findings, none were required for this PR; they're tracked in the review report for a future pass if wanted, not filed as issues automatically.
Every commit is independently verified (build, full test suite,
cargo fmt --all -- --check,cargo clippy --all-features --all-targets -- -D warnings) — not just trusted from the implementing agent's own claim.Testing
cargo test --all-features --workspaceandcargo test --test kittest --all-featuresboth green at HEAD (2257 lib tests, 316 kittest tests, 0 failures).cargo fmt --all -- --checkandcargo clippy --all-features --all-targets -- -D warningsboth clean.🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes