fix(identity): resolve a key's private half by matching material, not by deriving it from purpose (#889)

CHANGELOG.md

@@ -94,6 +94,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Fixed
 
+- **A saved voting key can now actually sign**: a voting key held on an
+  identity's own record — rather than on a separate voting identity — was saved
+  and shown as being on this device, but nothing could use it. Signing looked for
+  it in the wrong place, so voting with it failed and the key's page reported it
+  missing, on the screen whose job is to answer that question. Dash Evo Tool now
+  finds a key by matching it against the key itself, wherever it is filed, so it
+  is found whichever version of the app saved it and no key material has to be
+  moved to fix this. This also means a key is no longer confused with a different
+  key that happens to share its number, which a masternode has whenever its
+  voting identity numbers a key the same way as its main identity: removing one
+  key could remove the other's private half, and a key could be reported as
+  saved on the strength of an unrelated key being present.
+
 - **An identity's keys are reachable again**: the keys list under an identity's
   Settings → Advanced now opens each key's own page, so keys can be inspected
   and restored — and, once a key is on this device, signed with or

docs/ai-design/2026-07-30-key-placement-resolution/design.md

@@ -0,0 +1,162 @@
+# Key placement resolution
+
+How Dash Evo Tool decides which key store an identity key's private half lives
+in, and why that is asked of the store rather than derived from the key.
+
+Issue #889 follow-up. Supersedes the reconciliation-migration approach that was
+designed for this problem and then withdrawn — see §6.
+
+## 1. The defect
+
+An identity's private halves live in a `BTreeMap` keyed by
+`(PrivateKeyTarget, KeyID)`, serialized into the on-disk `QualifiedIdentity`
+blob. The target used to be derived from the key's `Purpose`:
+
+```rust
+Purpose::VOTING => PrivateKeyOnVoterIdentity,
+_               => PrivateKeyOnMainIdentity,
+```
+
+That derivation cannot express a `Purpose::VOTING` 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.
+
+For such a key, `sign` and `can_sign_with` looked in a store it was never filed
+under. The app accepted the key, saved it, listed it as held, and no signing path
+could find it. Two further consequences followed from the same derivation:
+
+* A delete could land on a **different key**. The voter and main key-id spaces
+  overlap, so id 0 names two keys on a masternode; removing one could remove the
+  other's private half.
+* A held check could pass on the strength of an unrelated key, because a probe
+  keyed on the id alone cannot tell two keys apart.
+
+## 2. Two questions, two functions
+
+The derivation conflated two questions that have different answers.
+
+| Question | Answer | Used by |
+|---|---|---|
+| Where **is** this key's private half filed? | `KeyStorage::candidates` — probes each store at the key's id, keeping only entries whose stored public-key data matches | every read and delete |
+| Where **should** a new private half go? | `QualifiedIdentity::placement_of` — reads the identity's own on-chain key lists | the write path only |
+
+`candidates` is three `BTreeMap` probes, not a scan, in a fixed
+[`PROBE_ORDER`] — so resolution never depends on map iteration order. It accepts
+an entry only when `same_key` does: every field of the public half except
+`disabled_at`, which is the one field Platform lets move after a key is added, so
+a key disabled on chain since it was saved still matches its stored snapshot.
+Matching on key *material* alone would not be enough — 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, and conflating them
+would hand out or delete material the requested key does not own.
+
+`placement_of` returns `Resolved` / `Ambiguous` / `Unknown`. `Unknown` is a real
+state, not a failure: `add_key_to_identity` inserts a key before broadcasting the
+transition that publishes it, so a key on no list is the steady state there.
+
+## 3. Resolve to bytes, not to a match
+
+`resolve_private_key_bytes` takes the public key and returns the first placement
+that **yields bytes** — not the first that matches.
+
+The difference matters for one specific shape: a `PrivateKeyData::InVault`
+placeholder whose vault secret is gone, sitting beside a live entry for the same
+key under another store. A resolver that stopped at the first match would report
+that key unusable with its bytes one probe away. Falling through makes the dead
+entry self-healing at read time, with nothing deleted.
+
+With nothing to fall through to, the first failure is returned rather than
+`Ok(None)`, so "the vault is not open" never degrades into "you never had that
+key".
+
+## 4. The map key and the vault label are one address
+
+A vault-backed key's bytes live under the label
+`identity_key_priv.<m|v|o>.<key_id>`, derived from the map key and scoped to the
+main identity id. Map key and label are therefore halves of one composite
+address: naming a store the blob does not agree with names a label the bytes were
+never stored under.
+
+`resolve_private_key_bytes` discovers the placement instead of accepting one, and
+builds the vault scope from what it found. With that signature a caller **cannot**
+pass a mismatched target. This is why the function takes an `IdentityPublicKey`
+rather than a `(target, key_id)` pair.
+
+## 5. Placement is not derivation
+
+Material matching fixes *where a key is filed*. It does not validate that a
+`PrivateKeyData::AtWalletDerivationPath` entry's stored path still derives the
+right key — an entry can be correctly matched here and still carry a stale path.
+That is what the `ECDSA_HASH160` recovery scan in `sign` exists for, and the two
+mechanisms are independent. Neither subsumes the other.
+
+## 6. Why no migration
+
+Both conventions exist on disk: `load_identity` and `add_key_to_identity` have
+always written structurally, while the Key Info paste path wrote purpose-derived.
+The obvious repair is a reconciliation pass that collapses to one convention.
+
+It was designed and rejected. With the material lookup permanent, the resolver
+does all of the correctness work, and moving entries buys only one convention on
+disk — hygiene. Against that:
+
+* Moving a vault-backed key is a **decrypt/re-encrypt**, because the AEAD binds
+  its AAD to `wallet_id ‖ label`. A Tier-2 (password-protected) key therefore
+  cannot be moved at boot at all, requiring a deferred password-gated flow.
+* The bytes are irreplaceable. An imported masternode voting or owner key has no
+  seed to regenerate from; a delete-after-copy that picks the wrong winner
+  destroys the only copy. `Cargo.toml`'s `bincode` pin reasons the same way about
+  the blob format.
+* The eventual move to keying by `(Identifier, KeyID)` changes the label anyway,
+  so collapsing now pays a destructive pass over secrets twice.
+
+Leaving both conventions in place costs one three-probe lookup and moves nothing.
+A crash cannot strand a key because nothing is ever in motion.
+
+## 7. Consequences
+
+* `impl From<Purpose> for PrivateKeyTarget` is **deleted**. It compiled away with
+  no fallout — evidence the derivation was fully contained — and its absence is
+  what stops a second derivation being reintroduced.
+* `KeyInfoScreen` no longer carries a `target` field. It resolves on demand, so
+  there is no state to thread through constructors and nothing for the
+  `ScreenType` round trip to drop.
+* `KeyStorage.private_keys` is **private**. With correctness concentrated in the
+  resolver, a caller reaching past it can silently miss a key that is present.
+  Explicit-placement accessors remain for callers that legitimately know one — a
+  loader walking the 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.
+
+## 8. What is not covered
+
+* **A deleted key's vault secret is not removed** (`key_info_screen.rs`, the
+  remove-private-key dialog): the map entry goes, the vault entry stays, so bytes
+  the user believes deleted remain on disk with nothing pointing at them. Its own
+  fix, with its own ordering argument (vault first, then map) and its own review.
+* **Keying by `(Identifier, KeyID)`** — the right end state, since it removes the
+  role enum entirely. Mechanical once the store is known-consistent.
+* Validating the signing path's vault-resolved key against the requested public
+  key, deferred separately.
+
+## 9. Test coverage
+
+| Test | Pins |
+|---|---|
+| `a_held_voting_key_on_the_main_identity_is_signable_under_either_placement` | the regression lock — a saved key must be usable under **both** placements, so writer and reader can never drift apart again |
+| `voting_key_on_the_main_identity_is_found_where_the_loader_files_it` | the defect: the shape the authoritative loader writes |
+| `a_voting_key_an_older_build_filed_under_voter_stays_findable` | the no-migration constraint |
+| `an_authentication_key_on_the_voter_identity_is_found` | the mirror defect |
+| `two_different_keys_sharing_an_id_are_never_confused` | the id collision |
+| `two_keys_sharing_id_and_material_are_told_apart_by_purpose` | the collision `data` alone cannot resolve |
+| `a_key_disabled_since_it_was_saved_is_still_found` | `disabled_at` is the one field that legitimately moves |
+| `removing_one_key_leaves_a_different_key_sharing_its_id_alone` | the delete path: confirmed RED against the purpose-derived removal, which left the key the user asked to delete in place and removed another |
+| `a_dead_vault_placeholder_falls_through_to_a_live_placement` | the fallthrough rule (§3) |
+| `a_lone_dead_placement_surfaces_its_error_rather_than_absence` | the other half of it |
+| `duplicate_placements_are_returned_in_probe_order` | determinism, not iteration order |
+| `an_entry_whose_material_disagrees_is_not_a_candidate` | the assumption material matching rests on |
+| `an_operator_filed_key_from_a_legacy_blob_stays_reachable` | `PrivateKeyOnOperatorIdentity` has no live writer but is legacy-reachable |
+| `both_keys_of_a_real_v093_blob_resolve_to_their_own_material` | a **real** v0.9.3 blob's keys are reachable, not merely decodable |
+
+[`PROBE_ORDER`]: ../../../src/model/qualified_identity/key_placement.rs

src/backend_task/dashpay/auto_accept_proof.rs

@@ -3,7 +3,6 @@ use crate::model::dashpay_derivation::derive_auto_accept_key;
 use crate::model::qualified_identity::QualifiedIdentity;
 use dash_sdk::dpp::dashcore::secp256k1::{Message, Secp256k1, SecretKey};
 use dash_sdk::dpp::identity::accessors::IdentityGettersV0;
-use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0;
 use dash_sdk::dpp::identity::{KeyType, Purpose, SecurityLevel};
 use dash_sdk::platform::Identifier;
 use serde::{Deserialize, Serialize};
@@ -153,10 +152,7 @@ pub async fn generate_auto_accept_proof(
     // Resolve the ENCRYPTION private key through the JIT chokepoint — no
     // parked-seed read.
     let wallet_seed = identity
-        .resolve_private_key_bytes(
-            crate::model::qualified_identity::PrivateKeyTarget::PrivateKeyOnMainIdentity,
-            signing_key.id(),
-        )
+        .resolve_private_key_bytes(signing_key)
         .await?
         .map(|(_, private_key)| private_key)
         .ok_or(TaskError::WalletLocked)?;
@@ -296,10 +292,7 @@ pub async fn verify_auto_accept_proof(
     // Resolve the ENCRYPTION private key through the JIT chokepoint — no
     // parked-seed read.
     let wallet_seed = our_identity
-        .resolve_private_key_bytes(
-            crate::model::qualified_identity::PrivateKeyTarget::PrivateKeyOnMainIdentity,
-            signing_key.id(),
-        )
+        .resolve_private_key_bytes(signing_key)
         .await
         .map_err(|e| format!("Error resolving private key: {}", e))?
         .map(|(_, private_key)| private_key)

src/backend_task/dashpay/contact_requests.rs

@@ -346,10 +346,7 @@ pub async fn send_contact_request_with_proof(
     // Resolve the ENCRYPTION private key through the JIT chokepoint — no
     // parked-seed read.
     let sender_private_key = identity
-        .resolve_private_key_bytes(
-            crate::model::qualified_identity::PrivateKeyTarget::PrivateKeyOnMainIdentity,
-            sender_encryption_key.id(),
-        )
+        .resolve_private_key_bytes(sender_encryption_key)
         .await?
         .map(|(_, private_key)| private_key)
         .ok_or_else(|| {

src/backend_task/dashpay/payments.rs

@@ -164,10 +164,7 @@ pub async fn derive_contact_payment_address(
 
     // Resolve our private key through the JIT chokepoint (no parked-seed read).
     let our_private_key = our_identity
-        .resolve_private_key_bytes(
-            crate::model::qualified_identity::PrivateKeyTarget::PrivateKeyOnMainIdentity,
-            our_key.id(),
-        )
+        .resolve_private_key_bytes(our_key)
         .await
         .map_err(|e| format!("Error resolving private key: {}", e))?
         .map(|(_, private_key)| private_key)

src/backend_task/grovestark.rs

@@ -1,7 +1,7 @@
 use crate::backend_task::BackendTaskSuccessResult;
 use crate::backend_task::error::TaskError;
 use crate::model::grovestark_prover::{ProofDataOutput, ProofMetadata, PublicInputsData};
-use crate::model::qualified_identity::{PrivateKeyTarget, QualifiedIdentity};
+use crate::model::qualified_identity::QualifiedIdentity;
 use dash_sdk::Sdk;
 use dash_sdk::dpp::document::DocumentV0Getters;
 use dash_sdk::dpp::identifier::Identifier;
@@ -37,8 +37,16 @@ pub async fn run_grovestark_task(
             // read), then derive its ed25519 public key. EDDSA_25519_HASH160
             // stores only the 20-byte hash on Platform, so the verifying key is
             // recovered from the resolved private key rather than read back.
+            //
+            // The key id is resolved to the identity's own published key first,
+            // so a request naming a key this identity does not have fails here
+            // rather than reaching the vault.
+            let signing_key = identity
+                .identity
+                .get_public_key_by_id(key_id)
+                .ok_or(TaskError::WalletKeyLookupFailed)?;
             let (_, private_key) = identity
-                .resolve_private_key_bytes(PrivateKeyTarget::PrivateKeyOnMainIdentity, key_id)
+                .resolve_private_key_bytes(signing_key)
                 .await?
                 .ok_or(TaskError::WalletKeyLookupFailed)?;
 

src/backend_task/identity/load_identity.rs

@@ -52,8 +52,8 @@ type WalletMatchResult = Option<(WalletSeedHash, u32, WalletKeyMap)>;
 /// but is NOT valid for the background legacy migration, where an absent field
 /// can be a deliberate removal (a cleared alias, a "Remove private key from DET").
 fn merge_existing_keys_into(new: &mut QualifiedIdentity, existing: QualifiedIdentity) {
-    for (key, value) in existing.private_keys.private_keys {
-        new.private_keys.private_keys.entry(key).or_insert(value);
+    for (key, value) in existing.private_keys.into_entries() {
+        new.private_keys.insert_if_absent(key, value);
     }
     if new.alias.is_none() {
         new.alias = existing.alias;
@@ -830,21 +830,21 @@ mod tests {
         let voter = IdentityPublicKey::random_key(2, Some(2), pv);
         let id_key = IdentityPublicKey::random_key(3, Some(3), pv);
         let triple = [(M, owner.id()), (V, voter.id()), (M, id_key.id())];
-        ks.private_keys.insert(
+        ks.insert_at(
             (M, owner.id()),
             (
                 QualifiedIdentityPublicKey::from(owner),
                 PrivateKeyData::Clear([0xA0; 32]),
             ),
         );
-        ks.private_keys.insert(
+        ks.insert_at(
             (V, voter.id()),
             (
                 QualifiedIdentityPublicKey::from(voter),
                 PrivateKeyData::Clear([0xB0; 32]),
             ),
         );
-        ks.private_keys.insert(
+        ks.insert_at(
             (M, id_key.id()),
             (
                 QualifiedIdentityPublicKey::from(id_key),
@@ -969,11 +969,10 @@ mod tests {
         // new value wins on collision.
         let (voter_pk, _) = existing
             .private_keys
-            .private_keys
-            .get(&voter_key)
+            .entry_at(&voter_key)
             .expect("existing voter key")
             .clone();
-        new.private_keys.private_keys.insert(
+        new.private_keys.insert_at(
             voter_key.clone(),
             (voter_pk, PrivateKeyData::Clear([0xEE; 32])),
         );
@@ -982,18 +981,17 @@ mod tests {
 
         // Owner and identity-auth keys survive the voter-key-only update.
         assert!(
-            new.private_keys.private_keys.contains_key(&owner_key),
+            new.private_keys.has(&owner_key),
             "owner key must survive a voting-key-only update",
         );
         assert!(
-            new.private_keys.private_keys.contains_key(&idkey_key),
+            new.private_keys.has(&idkey_key),
             "identity-auth key must survive a voting-key-only update",
         );
         // The resupplied voting key wins on collision (0xEE, not the old 0xB0).
         let (_, merged_voter) = new
             .private_keys
-            .private_keys
-            .get(&voter_key)
+            .entry_at(&voter_key)
             .expect("voter key present after merge");
         assert!(
             matches!(merged_voter, PrivateKeyData::Clear(b) if *b == [0xEE; 32]),
@@ -1067,10 +1065,7 @@ mod tests {
             .expect("first node still stored");
         for (t, k) in &triple {
             assert!(
-                still
-                    .private_keys
-                    .private_keys
-                    .contains_key(&(t.clone(), *k)),
+                still.private_keys.has(&(t.clone(), *k)),
                 "key ({t:?}, {k}) of the first node must survive a rejected duplicate load",
             );
         }
@@ -1138,7 +1133,7 @@ mod tests {
         let new_voter = IdentityPublicKey::random_key(9, Some(9), pv);
         let new_voter_id = new_voter.id();
         let new_key = (V, new_voter_id);
-        existing.private_keys.private_keys.insert(
+        existing.private_keys.insert_at(
             new_key.clone(),
             (
                 QualifiedIdentityPublicKey::from(new_voter),
@@ -1165,7 +1160,7 @@ mod tests {
         // The new key flipped to InVault in the in-memory identity...
         assert!(
             matches!(
-                existing.private_keys.private_keys.get(&new_key),
+                existing.private_keys.entry_at(&new_key),
                 Some((_, PrivateKeyData::InVault)),
             ),
             "the merged voting key must be marked InVault after sealing",