diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cada13c2..45a74636d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/ai-design/2026-07-30-key-placement-resolution/design.md b/docs/ai-design/2026-07-30-key-placement-resolution/design.md new file mode 100644 index 000000000..dc3c2a8e8 --- /dev/null +++ b/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..`, 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 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 diff --git a/src/backend_task/dashpay/auto_accept_proof.rs b/src/backend_task/dashpay/auto_accept_proof.rs index 9e8f227da..71bbd9a76 100644 --- a/src/backend_task/dashpay/auto_accept_proof.rs +++ b/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) diff --git a/src/backend_task/dashpay/contact_requests.rs b/src/backend_task/dashpay/contact_requests.rs index 59820909b..165840741 100644 --- a/src/backend_task/dashpay/contact_requests.rs +++ b/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(|| { diff --git a/src/backend_task/dashpay/payments.rs b/src/backend_task/dashpay/payments.rs index 469e80cf0..31db1e4de 100644 --- a/src/backend_task/dashpay/payments.rs +++ b/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) diff --git a/src/backend_task/grovestark.rs b/src/backend_task/grovestark.rs index 19498e528..ff12b16fd 100644 --- a/src/backend_task/grovestark.rs +++ b/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)?; diff --git a/src/backend_task/identity/load_identity.rs b/src/backend_task/identity/load_identity.rs index 6c00d4864..f5a0668b0 100644 --- a/src/backend_task/identity/load_identity.rs +++ b/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", diff --git a/src/backend_task/identity/protect_identity_keys.rs b/src/backend_task/identity/protect_identity_keys.rs index b35487a59..08b7470f9 100644 --- a/src/backend_task/identity/protect_identity_keys.rs +++ b/src/backend_task/identity/protect_identity_keys.rs @@ -565,7 +565,7 @@ mod tests { let pv = PlatformVersion::latest(); let mut ks = KeyStorage::default(); let k = IdentityPublicKey::random_key(1, Some(1), pv); - ks.private_keys.insert( + ks.insert_at( (M, k.id()), ( QualifiedIdentityPublicKey::from(k), @@ -582,7 +582,7 @@ mod tests { let pv = PlatformVersion::latest(); let mut ks = KeyStorage::default(); let k = IdentityPublicKey::random_key(1, Some(1), pv); - ks.private_keys.insert( + ks.insert_at( (M, k.id()), ( QualifiedIdentityPublicKey::from(k), @@ -599,7 +599,7 @@ mod tests { let pv = PlatformVersion::latest(); let mut ks = KeyStorage::default(); let vaulted = IdentityPublicKey::random_key(1, Some(1), pv); - ks.private_keys.insert( + ks.insert_at( (M, vaulted.id()), ( QualifiedIdentityPublicKey::from(vaulted), @@ -607,7 +607,7 @@ mod tests { ), ); let derived = IdentityPublicKey::random_key(2, Some(2), pv); - ks.private_keys.insert( + ks.insert_at( (M, derived.id()), ( QualifiedIdentityPublicKey::from(derived), @@ -627,7 +627,7 @@ mod tests { let pv = PlatformVersion::latest(); let mut ks = KeyStorage::default(); let a = IdentityPublicKey::random_key(1, Some(1), pv); - ks.private_keys.insert( + ks.insert_at( (M, a.id()), ( QualifiedIdentityPublicKey::from(a), @@ -635,7 +635,7 @@ mod tests { ), ); let b = IdentityPublicKey::random_key(2, Some(2), pv); - ks.private_keys.insert( + ks.insert_at( (M, b.id()), ( QualifiedIdentityPublicKey::from(b), @@ -643,7 +643,7 @@ mod tests { ), ); let derived = IdentityPublicKey::random_key(3, Some(3), pv); - ks.private_keys.insert( + ks.insert_at( (M, derived.id()), ( QualifiedIdentityPublicKey::from(derived), diff --git a/src/backend_task/identity/recover_legacy_keys.rs b/src/backend_task/identity/recover_legacy_keys.rs index 697a0b24b..8dd6a5269 100644 --- a/src/backend_task/identity/recover_legacy_keys.rs +++ b/src/backend_task/identity/recover_legacy_keys.rs @@ -582,7 +582,7 @@ mod tests { publish_on(&mut identity, published); for (target, key, data) in held { publish_on(&mut identity, &[key]); - private_keys.private_keys.insert( + private_keys.insert_at( (target, key.id()), (QualifiedIdentityPublicKey::from(key.public.clone()), data), ); @@ -1583,7 +1583,7 @@ mod tests { "the update that landed during the prompt must survive the restore", ); assert!( - stored.private_keys.private_keys.contains_key(&(M, 2)), + stored.private_keys.has(&(M, 2)), "and the restored key must still be there", ); diff --git a/src/backend_task/identity/withdraw_from_identity.rs b/src/backend_task/identity/withdraw_from_identity.rs index 5307f9036..df927b999 100644 --- a/src/backend_task/identity/withdraw_from_identity.rs +++ b/src/backend_task/identity/withdraw_from_identity.rs @@ -84,7 +84,7 @@ impl AppContext { .map_err(|_| TaskError::OwnerKeyWithdrawalNotAllowed)?; tracing::debug!( - num_private_keys = qualified_identity.private_keys.private_keys.len(), + num_private_keys = qualified_identity.private_keys.len(), num_wallets = qualified_identity.associated_wallets.len(), "Qualified identity key info" ); diff --git a/src/backend_task/migration/v093_upgrade.rs b/src/backend_task/migration/v093_upgrade.rs index 9905c5896..0413b05cc 100644 --- a/src/backend_task/migration/v093_upgrade.rs +++ b/src/backend_task/migration/v093_upgrade.rs @@ -758,7 +758,6 @@ fn stored_key_data(stored: &StoredIdentityOnDisk) -> Vec { QualifiedIdentity::from_bytes(&stored.qi_bytes) .expect("the stored blob must decode") .private_keys - .private_keys .values() .map(|(_, data)| data.clone()) .collect() @@ -1131,16 +1130,16 @@ fn a_real_v093_identity_blob_still_decodes() { // The keys are the payload. v0.9.3 held them `Clear`, and they must decode // to exactly the bytes it wrote — a shifted field or a changed varint would // corrupt them silently. - let keys = &qi.private_keys.private_keys; + let keys = &qi.private_keys; assert_eq!(keys.len(), 2, "both private keys must decode"); assert_eq!( - keys.get(&(PrivateKeyTarget::PrivateKeyOnMainIdentity, 0)) + keys.entry_at(&(PrivateKeyTarget::PrivateKeyOnMainIdentity, 0)) .map(|(_, data)| data.clone()), Some(PrivateKeyData::Clear(OWNER_PRIVATE_KEY)), "the masternode owner key must decode byte-for-byte", ); assert_eq!( - keys.get(&(PrivateKeyTarget::PrivateKeyOnVoterIdentity, 1)) + keys.entry_at(&(PrivateKeyTarget::PrivateKeyOnVoterIdentity, 1)) .map(|(_, data)| data.clone()), Some(PrivateKeyData::Clear(VOTING_PRIVATE_KEY)), "the masternode voting key must decode byte-for-byte", @@ -1156,6 +1155,57 @@ fn a_real_v093_identity_blob_still_decodes() { ); } +/// The same real v0.9.3 blob, but asking the question a user asks: are these +/// keys *usable* after the upgrade? +/// +/// Decoding proves the bytes survived; it does not prove anything can find them. +/// Both of this blob's keys are shapes the retired purpose derivation placed +/// wrongly — an `OWNER` key filed on the main identity derived to `Main` and was +/// fine, but a `VOTING` key filed on the **voter** identity is only reachable +/// because that is where v0.9.3 put it, and the structural answer for a key the +/// voter identity publishes is also `Voter`. Pinning both here means a future +/// change to placement cannot strand a real install's masternode keys without +/// failing this test. +#[test] +fn both_keys_of_a_real_v093_blob_resolve_to_their_own_material() { + use dash_sdk::dpp::identity::accessors::IdentityGettersV0; + use dash_sdk::dpp::identity::signer::Signer; + + let qi = QualifiedIdentity::from_bytes(&v093_masternode_blob()) + .expect("a genuine v0.9.3 identity blob must decode on the current bincode"); + + let owner = qi + .identity + .get_public_key_by_id(0) + .expect("the owner key is published on the main identity"); + assert_eq!( + qi.private_keys.candidates(owner).collect::>(), + vec![(PrivateKeyTarget::PrivateKeyOnMainIdentity, 0)], + "the owner key resolves to the store v0.9.3 filed it under", + ); + assert!( + qi.can_sign_with(owner), + "a masternode upgraded from v0.9.3 must still be able to sign with its owner key", + ); + + let (voter_identity, _) = qi + .associated_voter_identity + .as_ref() + .expect("the blob carries a voter identity"); + let voting = voter_identity + .get_public_key_by_id(1) + .expect("the voting key is published on the voter identity"); + assert_eq!( + qi.private_keys.candidates(voting).collect::>(), + vec![(PrivateKeyTarget::PrivateKeyOnVoterIdentity, 1)], + "the voting key resolves to the store v0.9.3 filed it under", + ); + assert!( + qi.can_sign_with(voting), + "a masternode upgraded from v0.9.3 must still be able to vote", + ); +} + /// The second launch after an upgrade. Every step must short-circuit on its /// sentinel: no duplicated votes, no resurrected history, no rewritten sentinel, /// no clobbered preferences — and the legacy rows still in `data.db`, because a diff --git a/src/context/identity_db.rs b/src/context/identity_db.rs index bbf7225c8..2cab1b96c 100644 --- a/src/context/identity_db.rs +++ b/src/context/identity_db.rs @@ -1815,7 +1815,7 @@ mod tests { let pv = PlatformVersion::latest(); let mut ks = KeyStorage::default(); let high = IdentityPublicKey::random_key(1, Some(1), pv); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, high.id()), ( QualifiedIdentityPublicKey::from(high), @@ -1823,7 +1823,7 @@ mod tests { ), ); let medium = IdentityPublicKey::random_key(2, Some(2), pv); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, medium.id()), ( QualifiedIdentityPublicKey::from(medium), @@ -1831,7 +1831,7 @@ mod tests { ), ); let derived = IdentityPublicKey::random_key(3, Some(3), pv); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, derived.id()), ( QualifiedIdentityPublicKey::from(derived), @@ -1887,14 +1887,10 @@ mod tests { ); // And the in-memory blob being persisted is already InVault-only. assert!( - migrated - .private_keys - .private_keys - .values() - .all(|(_, d)| !matches!( - d, - PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_) - )), + migrated.private_keys.values().all(|(_, d)| !matches!( + d, + PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_) + )), "persisted blob must carry no plaintext" ); persisted = true; @@ -1927,7 +1923,7 @@ mod tests { ); // KeyStorage now has zero Clear/AlwaysClear; the derived key remains. let mut derived = 0; - for (_, d) in qi.private_keys.private_keys.values() { + for (_, d) in qi.private_keys.values() { match d { PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_) => { panic!("plaintext survived migration") @@ -2068,7 +2064,7 @@ mod tests { let pv = PlatformVersion::latest(); let mut ks = KeyStorage::default(); let existing = IdentityPublicKey::random_key(1, Some(1), pv); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, existing.id()), ( QualifiedIdentityPublicKey::from(existing), @@ -2077,7 +2073,7 @@ mod tests { ); let added = IdentityPublicKey::random_key(2, Some(2), pv); let added_id = added.id(); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, added_id), ( QualifiedIdentityPublicKey::from(added), @@ -2211,7 +2207,7 @@ mod tests { // Decoding the stored blob yields no plaintext key variant at all. let decoded = QualifiedIdentity::from_bytes(&blob).expect("decode"); - for (_, d) in decoded.private_keys.private_keys.values() { + for (_, d) in decoded.private_keys.values() { assert!( !matches!(d, PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_)), "persisted write-path blob must carry no plaintext key", @@ -2238,7 +2234,6 @@ mod tests { // The caller's in-memory identity keeps its resident keys (signing still // works this session) — the encoder operates on a clone. let clear_in_caller = qi - .private_keys .private_keys .values() .filter(|(_, d)| matches!(d, PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_))) @@ -2315,7 +2310,7 @@ mod tests { let mut ks = KeyStorage::default(); let pv = PlatformVersion::latest(); let pk = IdentityPublicKey::random_key(0, Some(0), pv); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, 0), ( QualifiedIdentityPublicKey::from(pk), diff --git a/src/context/wallet_lifecycle/tests.rs b/src/context/wallet_lifecycle/tests.rs index de0af38f5..d881461e1 100644 --- a/src/context/wallet_lifecycle/tests.rs +++ b/src/context/wallet_lifecycle/tests.rs @@ -2231,7 +2231,7 @@ async fn clear_network_database_wipes_local_identity_private_keys() { let key = IdentityPublicKey::random_key(1, Some(1), pv); let key_id = key.id(); let mut private_keys = KeyStorage::default(); - private_keys.private_keys.insert( + private_keys.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, key_id), ( QualifiedIdentityPublicKey::from(key), @@ -2348,7 +2348,7 @@ async fn clear_network_database_reports_incomplete_when_masternode_key_delete_fa for (key_id, purpose, target) in key_specs { let mut key = IdentityPublicKey::random_key(key_id, Some(1), pv); key.set_purpose(purpose); - private_keys.private_keys.insert( + private_keys.insert_at( (target, key.id()), ( QualifiedIdentityPublicKey::from(key), @@ -3610,9 +3610,7 @@ async fn rediscovery_update_preserves_user_alias_and_wallet_binding() { associated_owner_key_id: None, identity_type: IdentityType::User, alias: alias.map(str::to_string), - private_keys: KeyStorage { - private_keys: BTreeMap::new(), - }, + private_keys: KeyStorage::default(), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, diff --git a/src/mcp/tools/masternode.rs b/src/mcp/tools/masternode.rs index 8f28e3e50..eb210dc69 100644 --- a/src/mcp/tools/masternode.rs +++ b/src/mcp/tools/masternode.rs @@ -813,7 +813,7 @@ mod tests { let (owner_key, owner_secret) = IdentityPublicKey::random_masternode_owner_key(0, Some(1), pv).expect("owner key"); public_keys.insert(owner_key.id(), owner_key.clone()); - key_storage.private_keys.insert( + key_storage.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, owner_key.id()), ( QualifiedIdentityPublicKey::from(owner_key), @@ -826,7 +826,7 @@ mod tests { IdentityPublicKey::random_masternode_transfer_key(1, Some(2), pv) .expect("transfer key"); public_keys.insert(payout_key.id(), payout_key.clone()); - key_storage.private_keys.insert( + key_storage.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, payout_key.id()), ( QualifiedIdentityPublicKey::from(payout_key), diff --git a/src/model/legacy_recovery.rs b/src/model/legacy_recovery.rs index a1b89ee3d..86fd6a06f 100644 --- a/src/model/legacy_recovery.rs +++ b/src/model/legacy_recovery.rs @@ -249,8 +249,8 @@ pub fn compute_recovery_plan( ) -> RecoveryPlan { let mut plan = RecoveryPlan::default(); - for (map_key, (public_key, data)) in &legacy.private_keys.private_keys { - if modern.private_keys.private_keys.contains_key(map_key) { + for (map_key, (public_key, data)) in legacy.private_keys.iter() { + if modern.private_keys.has(map_key) { continue; } let (target, key_id) = map_key.clone(); @@ -393,16 +393,12 @@ pub fn apply_recovery_plan( match &descriptor.item { RecoveryItem::Key { target, key_id } => { let map_key = (target.clone(), *key_id); - let Some(entry) = legacy.private_keys.private_keys.remove(&map_key) else { + let Some(entry) = legacy.private_keys.remove_at(&map_key) else { continue; }; // `or_insert`, never `insert`: the modern record wins every // collision by construction, not by the caller being careful. - merged - .private_keys - .private_keys - .entry(map_key) - .or_insert(entry); + merged.private_keys.insert_if_absent(map_key, entry); } RecoveryItem::VoterAssociation => { if merged.associated_voter_identity.is_none() { @@ -533,7 +529,6 @@ fn reference_identity<'a>( /// signals [`QualifiedIdentity::masternode_key_presence`] reads. fn holds_a_voting_key(record: &QualifiedIdentity) -> bool { record - .private_keys .private_keys .iter() .any(|((target, _), (public_key, _))| { @@ -656,7 +651,7 @@ mod tests { key: &TestKey, data: PrivateKeyData, ) { - qi.private_keys.private_keys.insert( + qi.private_keys.insert_at( (target, key.id()), (QualifiedIdentityPublicKey::from(key.public.clone()), data), ); @@ -721,8 +716,7 @@ mod tests { fn key_data(qi: &QualifiedIdentity, target: PrivateKeyTarget, key_id: KeyID) -> PrivateKeyData { qi.private_keys - .private_keys - .get(&(target, key_id)) + .entry_at(&(target, key_id)) .expect("key present") .1 .clone() @@ -872,12 +866,7 @@ mod tests { let applied = apply_recovery_plan(&modern, legacy, &[key_item(M, 1)]); assert!( - applied.applied.is_empty() - && !applied - .merged - .private_keys - .private_keys - .contains_key(&(M, 1)), + applied.applied.is_empty() && !applied.merged.private_keys.has(&(M, 1)), "an excluded key must never be merged, even if approved", ); } @@ -1013,15 +1002,15 @@ mod tests { let applied = apply_recovery_plan(&base, legacy, &plan.approved_items()); let merged = &applied.merged; - for (map_key, entry) in &base.private_keys.private_keys { + for (map_key, entry) in base.private_keys.iter() { assert_eq!( - merged.private_keys.private_keys.get(map_key), + merged.private_keys.entry_at(map_key), Some(entry), "every modern key must survive byte-identical", ); } assert!( - merged.private_keys.private_keys.len() >= base.private_keys.private_keys.len(), + merged.private_keys.len() >= base.private_keys.len(), "the merged key map must be a superset of the modern one", ); assert_eq!(merged.alias, base.alias, "alias has no write path here"); @@ -1076,11 +1065,7 @@ mod tests { assert!(applied.applied.is_empty()); assert_eq!(key_data(&applied.merged, M, 1), held.clear()); assert!( - !applied - .merged - .private_keys - .private_keys - .contains_key(&(M, 2)), + !applied.merged.private_keys.has(&(M, 2)), "a candidate the user did not approve must never be merged", ); } @@ -1141,7 +1126,7 @@ mod tests { // The pair is internally consistent; only the slot it sits in is wrong. let mut legacy = bare_identity(0x28); - legacy.private_keys.private_keys.insert( + legacy.private_keys.insert_at( (M, 7), ( QualifiedIdentityPublicKey::from(published.public.clone()), @@ -1171,7 +1156,7 @@ mod tests { let IdentityPublicKey::V0(mut v0) = live.public.clone(); v0.id = 5; let mut legacy = bare_identity(0x29); - legacy.private_keys.private_keys.insert( + legacy.private_keys.insert_at( (M, 5), ( QualifiedIdentityPublicKey::from(IdentityPublicKey::V0(v0)), diff --git a/src/model/qualified_identity/encrypted_key_storage.rs b/src/model/qualified_identity/encrypted_key_storage.rs index 25fe42699..7cf0a04c4 100644 --- a/src/model/qualified_identity/encrypted_key_storage.rs +++ b/src/model/qualified_identity/encrypted_key_storage.rs @@ -10,6 +10,7 @@ use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicK use dash_sdk::dpp::identity::{KeyID, Purpose, SecurityLevel}; use dash_sdk::dpp::key_wallet::bip32::ChildNumber; use dash_sdk::dpp::key_wallet::bip32::DerivationPath; +use dash_sdk::platform::IdentityPublicKey; use std::collections::{BTreeMap, BTreeSet}; use std::fmt; use std::sync::{Arc, RwLock}; @@ -20,6 +21,54 @@ use zeroize::Zeroizing; /// dropped. pub type ResolvedPrivateKey = (QualifiedIdentityPublicKey, Zeroizing<[u8; 32]>); +/// Whether a `stored` public half is the same key as the `live` one, ignoring +/// only `disabled_at`. +/// +/// Every field of an `IdentityPublicKey` is immutable once the key is added, with +/// that single exception: disabling a key rewrites it. The stored copy is a +/// snapshot taken when the private half was saved, so plain `==` stops matching +/// as soon as a key is disabled or rotated, and a key this device demonstrably +/// holds gets reported as missing. +/// +/// Comparing only the id and the key material would fix that and open a worse +/// hole the other way. 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 a lookup that conflates them can hand out, +/// or delete, material the requested key does not own. So this excludes the one +/// field that legitimately moves and nothing else. +pub fn same_key(stored: &IdentityPublicKey, live: &IdentityPublicKey) -> bool { + use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + + let IdentityPublicKey::V0(stored) = stored; + let IdentityPublicKey::V0(live) = live; + // Destructured exhaustively, and without `..`, on purpose: a field added + // upstream must break this build rather than be silently ignored. A new field + // that distinguishes two keys would otherwise leave this reporting a match + // where there is none — which is how one key's private material ends up + // attributed to another. Whoever adds it decides here whether it identifies a + // key or, like `disabled_at`, only describes its state. + let IdentityPublicKeyV0 { + id, + purpose, + security_level, + contract_bounds, + key_type, + read_only, + data, + // The one field Platform lets move after a key is added: disabling a key + // rewrites it, and the stored snapshot predates that. + disabled_at: _, + } = stored; + + *id == live.id + && *purpose == live.purpose + && *security_level == live.security_level + && *contract_bounds == live.contract_bounds + && *key_type == live.key_type + && *read_only == live.read_only + && *data == live.data +} + /// A `(target, key_id)` map key paired with the raw 32-byte private key the /// migration must store in the vault — see /// [`KeyStorage::take_plaintext_for_vault`]. Bytes are [`Zeroizing`]. @@ -198,10 +247,19 @@ impl fmt::Display for PrivateKeyData { } } +/// Every private half this install holds for one identity, keyed by the store it +/// is filed under and the key's id. +/// +/// The map is private on purpose. Which store a key is filed under is not +/// something a caller should be deriving for itself — that is what produced a +/// saved key no signing path could find — so reads go through +/// [`candidates`](Self::candidates), which selects on key material. The +/// remaining direct accessors exist for callers that legitimately name a +/// placement: a loader that knows structurally where a key belongs, and legacy +/// recovery, which is *about* specific stored placements. #[derive(Debug, Encode, Decode, Clone, PartialEq, Default)] pub struct KeyStorage { - pub private_keys: - BTreeMap<(PrivateKeyTarget, KeyID), (QualifiedIdentityPublicKey, PrivateKeyData)>, + private_keys: BTreeMap<(PrivateKeyTarget, KeyID), (QualifiedIdentityPublicKey, PrivateKeyData)>, } impl From> @@ -413,11 +471,132 @@ impl KeyStorage { self.private_keys.contains_key(key) } + /// Every map key whose stored public half *is* `key`, in + /// [`PROBE_ORDER`](crate::model::qualified_identity::key_placement::PROBE_ORDER). + /// + /// This is how a read finds a private half. It probes each store at `key`'s + /// own id and keeps only entries [`same_key`] accepts, so it finds the key + /// wherever an older build filed it and never returns a different key that + /// merely shares the id — the voter and main id spaces overlap, so matching + /// on the id alone is what lets a delete land on the wrong key. + /// + /// Three `BTreeMap` probes, not a scan. Yields more than one entry only when + /// the same key is genuinely filed under several stores; a caller that needs + /// bytes should take the first that *resolves* rather than the first that + /// matches, since a match can name a vault placeholder whose secret is gone + /// (see + /// [`resolve_private_key_bytes`](crate::model::qualified_identity::QualifiedIdentity::resolve_private_key_bytes)). + pub fn candidates<'a>( + &'a self, + key: &'a IdentityPublicKey, + ) -> impl Iterator + 'a { + let key_id = key.id(); + crate::model::qualified_identity::key_placement::PROBE_ORDER + .iter() + .filter_map(move |target| { + let map_key = (target.clone(), key_id); + let (stored, _) = self.private_keys.get(&map_key)?; + same_key(&stored.identity_public_key, key).then_some(map_key) + }) + } + /// Returns all stored key identifiers. pub fn keys_set(&self) -> BTreeSet<(PrivateKeyTarget, KeyID)> { self.private_keys.keys().cloned().collect() } + /// How many private halves are held. + pub fn len(&self) -> usize { + self.private_keys.len() + } + + /// Whether no private half is held at all. + pub fn is_empty(&self) -> bool { + self.private_keys.is_empty() + } + + /// Every entry, keyed by placement. Target-blind, for callers that need to + /// walk the whole store rather than find one key. + pub fn iter( + &self, + ) -> impl Iterator< + Item = ( + &(PrivateKeyTarget, KeyID), + &(QualifiedIdentityPublicKey, PrivateKeyData), + ), + > { + self.private_keys.iter() + } + + /// Every stored entry, without its placement. Target-blind, for callers + /// asking about the keys themselves rather than where they are filed. + pub fn values(&self) -> impl Iterator { + self.private_keys.values() + } + + /// The stored entry at exactly `key`, if any. + /// + /// Names a placement directly, so it answers "is *this* slot occupied", not + /// "where is this key". Prefer [`candidates`](Self::candidates) for the + /// latter: this cannot tell a key from a different one sharing its id. + pub fn entry_at( + &self, + key: &(PrivateKeyTarget, KeyID), + ) -> Option<&(QualifiedIdentityPublicKey, PrivateKeyData)> { + self.private_keys.get(key) + } + + /// Store `value` at exactly `key`, replacing whatever was there. + /// + /// For callers that know a placement structurally — a loader walking the + /// identity list it read a key from, or legacy recovery restoring an entry + /// to the placement the old blob recorded. Anything choosing a placement for + /// *new* material should take it from + /// [`QualifiedIdentity::placement_of`](crate::model::qualified_identity::QualifiedIdentity::placement_of). + pub fn insert_at( + &mut self, + key: (PrivateKeyTarget, KeyID), + value: (QualifiedIdentityPublicKey, PrivateKeyData), + ) -> Option<(QualifiedIdentityPublicKey, PrivateKeyData)> { + self.private_keys.insert(key, value) + } + + /// Store `value` at `key` only if nothing is there — the merge-preserving + /// write. Used when folding a previously-loaded record into a fresh one, so + /// a key the new load did not resupply is kept rather than dropped, and one + /// it did resupply is not overwritten with the stale copy. + pub fn insert_if_absent( + &mut self, + key: (PrivateKeyTarget, KeyID), + value: (QualifiedIdentityPublicKey, PrivateKeyData), + ) { + self.private_keys.entry(key).or_insert(value); + } + + /// Consume the store, yielding every entry with its placement. + pub fn into_entries( + self, + ) -> impl Iterator< + Item = ( + (PrivateKeyTarget, KeyID), + (QualifiedIdentityPublicKey, PrivateKeyData), + ), + > { + self.private_keys.into_iter() + } + + /// Remove the entry at exactly `key`, returning it if it was there. + /// + /// Removing *a key* rather than a slot means removing every placement that + /// holds it — see [`candidates`](Self::candidates), which selects on key + /// material so a removal cannot land on a different key sharing the id. + pub fn remove_at( + &mut self, + key: &(PrivateKeyTarget, KeyID), + ) -> Option<(QualifiedIdentityPublicKey, PrivateKeyData)> { + self.private_keys.remove(key) + } + pub fn identity_public_keys(&self) -> Vec<(&PrivateKeyTarget, &QualifiedIdentityPublicKey)> { self.private_keys .iter() diff --git a/src/model/qualified_identity/key_placement.rs b/src/model/qualified_identity/key_placement.rs new file mode 100644 index 000000000..eb915365a --- /dev/null +++ b/src/model/qualified_identity/key_placement.rs @@ -0,0 +1,63 @@ +//! Which key store an identity key's private half belongs to. +//! +//! Two different questions, deliberately answered by two different functions: +//! +//! * *Where is this key's private half filed?* — [`KeyStorage::candidates`], +//! which matches on the stored public-key material instead of trusting a +//! derived target. Every read and delete asks this one. +//! * *Where should a private half we are about to store go?* — +//! [`QualifiedIdentity::placement_of`], which reads the identity's own +//! on-chain key lists. Only the write path asks this one. +//! +//! [`KeyStorage::candidates`]: super::encrypted_key_storage::KeyStorage::candidates +//! [`QualifiedIdentity::placement_of`]: super::QualifiedIdentity::placement_of +//! +//! Deriving the store from a key's [`Purpose`](dash_sdk::dpp::identity::Purpose) +//! answers neither: a voting-purpose key filed on the main identity is a +//! supported shape, and no purpose distinguishes it from one on a voter +//! identity. + +use crate::model::qualified_identity::PrivateKeyTarget; + +/// Every store a private half can be filed under, in resolution order. +/// +/// The order fixes which candidate wins when a key legitimately matches more +/// than one, so resolution never depends on map iteration order. +/// [`PrivateKeyOnOperatorIdentity`](PrivateKeyTarget::PrivateKeyOnOperatorIdentity) +/// is included even though nothing in this app writes it any more: a v0.9.3-era +/// blob can carry one, and omitting the probe would stand those keys up as +/// unfindable. +pub const PROBE_ORDER: [PrivateKeyTarget; 3] = [ + PrivateKeyTarget::PrivateKeyOnMainIdentity, + PrivateKeyTarget::PrivateKeyOnVoterIdentity, + PrivateKeyTarget::PrivateKeyOnOperatorIdentity, +]; + +/// Which of an identity's key lists publishes a given key. +/// +/// This is the identity's *own on-chain record*, so it answers where a private +/// half should be filed — not where one already is. A key can be published and +/// still have its private half filed elsewhere by an older build; only +/// [`KeyStorage::candidates`](super::encrypted_key_storage::KeyStorage::candidates) +/// answers that. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum KeyPlacement { + /// Exactly one list publishes this key. + Resolved(PrivateKeyTarget), + /// More than one list publishes the same key under the same id. Nothing may + /// guess between them. + Ambiguous(Vec), + /// No list publishes it — normal for a key added locally whose state + /// transition has not been broadcast yet. + Unknown, +} + +impl KeyPlacement { + /// The single target, or `None` for `Ambiguous`/`Unknown`. + pub fn resolved(&self) -> Option { + match self { + KeyPlacement::Resolved(target) => Some(target.clone()), + _ => None, + } + } +} diff --git a/src/model/qualified_identity/mod.rs b/src/model/qualified_identity/mod.rs index 5a3d2a2b6..b5659d650 100644 --- a/src/model/qualified_identity/mod.rs +++ b/src/model/qualified_identity/mod.rs @@ -1,5 +1,6 @@ pub mod encrypted_key_storage; pub mod identity_meta; +pub mod key_placement; pub mod qualified_identity_public_key; // TODO(det): this upward edge is fixed by the `SecretAccess::with_secret` @@ -8,6 +9,7 @@ pub mod qualified_identity_public_key; // type — a wallet_backend change out of scope here. use crate::backend_task::error::TaskError; use crate::model::qualified_identity::encrypted_key_storage::{KeyStorage, ResolvedPrivateKey}; +use crate::model::qualified_identity::key_placement::KeyPlacement; use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; use crate::model::user_role::UserRole; use crate::model::wallet::{Wallet, WalletSeedHash}; @@ -117,6 +119,14 @@ pub struct OwnerKeyWithdrawalNotAllowed; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct NoUsableWithdrawalKey; +/// Which of an identity's key stores a private half is filed under. +/// +/// Deliberately has no conversion from [`Purpose`]: a key's purpose does not +/// determine its store, since a voting-purpose key filed on the main identity is +/// a supported shape. Ask +/// [`KeyStorage::candidates`](encrypted_key_storage::KeyStorage::candidates) +/// where a private half is, or [`QualifiedIdentity::placement_of`] where a new +/// one belongs. #[derive(Debug, Encode, Decode, Clone, Hash, Ord, PartialOrd, Eq, PartialEq)] #[allow(clippy::enum_variant_names)] pub enum PrivateKeyTarget { @@ -125,15 +135,6 @@ pub enum PrivateKeyTarget { PrivateKeyOnOperatorIdentity, } -impl From for PrivateKeyTarget { - fn from(value: Purpose) -> Self { - match value { - Purpose::VOTING => PrivateKeyTarget::PrivateKeyOnVoterIdentity, - _ => PrivateKeyTarget::PrivateKeyOnMainIdentity, - } - } -} - #[derive(Debug, Encode, Decode, Clone, PartialEq)] pub struct DPNSNameInfo { pub name: String, @@ -357,7 +358,6 @@ impl Signer for QualifiedIdentity { identity_public_key: &IdentityPublicKey, data: &[u8], ) -> Result { - let target: PrivateKeyTarget = identity_public_key.purpose().into(); let key_id = identity_public_key.id(); tracing::debug!( @@ -365,14 +365,14 @@ impl Signer for QualifiedIdentity { key_id = key_id, key_purpose = ?identity_public_key.purpose(), key_type = ?identity_public_key.key_type(), - target = ?target, "Attempting to sign with key" ); - // Resolve the signing key without ever reading a wallet's parked seed - // (see [`Self::resolve_private_key_bytes`]). + // Resolve the signing key wherever its private half is filed, without + // ever reading a wallet's parked seed (see + // [`Self::resolve_private_key_bytes`]). let resolved = self - .resolve_private_key_bytes(target.clone(), key_id) + .resolve_private_key_bytes(identity_public_key) .await .map_err(|e| ProtocolError::Generic(e.to_string()))?; @@ -380,12 +380,11 @@ impl Signer for QualifiedIdentity { tracing::error!( key_id = key_id, purpose = ?identity_public_key.purpose(), - target = ?target, "Key not found in identity" ); // Only dump the identity's available keys when resolution failed — // this is the diagnostic that actually matters, off the hot path. - for ((t, id), (pub_key, _)) in self.private_keys.private_keys.iter() { + for ((t, id), (pub_key, _)) in self.private_keys.iter() { tracing::debug!( target = ?t, key_id = id, @@ -472,11 +471,21 @@ impl Signer for QualifiedIdentity { } } + /// Whether a private half for `identity_public_key` is filed anywhere on + /// this identity. + /// + /// Synchronous, so it cannot reach the vault: it answers from the stored + /// placements alone. A key whose only placement is a vault placeholder whose + /// secret has gone therefore reads as signable here while + /// [`QualifiedIdentity::resolve_private_key_bytes`] — which actually fetches + /// bytes — reports it unusable. That asymmetry is deliberate: this is a + /// cheap per-frame predicate for enabling UI, and the resolver is the + /// authority. fn can_sign_with(&self, identity_public_key: &IdentityPublicKey) -> bool { - self.private_keys.has(&( - identity_public_key.purpose().into(), - identity_public_key.id(), - )) + self.private_keys + .candidates(identity_public_key) + .next() + .is_some() } async fn sign_create_witness( @@ -582,7 +591,7 @@ impl QualifiedIdentity { owner: false, payout: false, }; - for (public_key, _) in self.private_keys.private_keys.values() { + for (public_key, _) in self.private_keys.values() { match public_key.identity_public_key.purpose() { Purpose::VOTING => presence.voting = true, Purpose::OWNER => presence.owner = true, @@ -593,8 +602,74 @@ impl QualifiedIdentity { presence } - /// Resolve the 32-byte private key for `(target, key_id)` without ever - /// reading a wallet's parked seed. + /// Which of this identity's key lists publishes `key` — where a private + /// half for it should be filed. + /// + /// Reads the identity's own on-chain records (main, voter, operator), + /// matching on key id **and** public-key data, so a key id that appears on + /// two lists with different material cannot be confused. Used by the write + /// path only; a read asks + /// [`KeyStorage::candidates`](encrypted_key_storage::KeyStorage::candidates) + /// where the private half actually is, which is not always the same answer. + /// + /// [`KeyPlacement::Unknown`] is normal, not an error: a key added locally + /// is not on any list until its state transition is broadcast. + pub fn placement_of(&self, key: &IdentityPublicKey) -> KeyPlacement { + let lists = [ + ( + PrivateKeyTarget::PrivateKeyOnMainIdentity, + Some(&self.identity), + ), + ( + PrivateKeyTarget::PrivateKeyOnVoterIdentity, + self.associated_voter_identity + .as_ref() + .map(|(identity, _)| identity), + ), + ( + PrivateKeyTarget::PrivateKeyOnOperatorIdentity, + self.associated_operator_identity + .as_ref() + .map(|(identity, _)| identity), + ), + ]; + + let publishing: Vec = lists + .into_iter() + .filter_map(|(target, identity)| { + let published = identity?.public_keys().get(&key.id())?; + (published.data() == key.data()).then_some(target) + }) + .collect(); + + match publishing.len() { + 0 => KeyPlacement::Unknown, + 1 => KeyPlacement::Resolved( + publishing + .into_iter() + .next() + .expect("invariant: length checked to be 1"), + ), + _ => KeyPlacement::Ambiguous(publishing), + } + } + + /// Resolve the 32-byte private key for `key`, wherever its private half is + /// filed, without ever reading a wallet's parked seed. + /// + /// Walks [`KeyStorage::candidates`] and returns the first placement that + /// actually **yields bytes** — not merely the first that matches. That + /// distinction is the point: a match can name a + /// [`PrivateKeyData::InVault`] placeholder whose vault secret is gone, + /// sitting beside a live entry for the same key under another store. Taking + /// the first match would report such a key unusable while its bytes are one + /// probe away. + /// + /// Because the placement is discovered rather than supplied, the vault scope + /// is built from the store the bytes were **found** under. The map key and + /// the vault label are one composite address, so a caller cannot be trusted + /// to pass a target that agrees with the blob — and with this signature it + /// cannot pass one at all. /// /// A wallet-derived key ([`PrivateKeyData::AtWalletDerivationPath`]) pulls /// its HD seed just-in-time through the [`SecretAccess`] chokepoint and @@ -604,11 +679,51 @@ impl QualifiedIdentity { /// decides which path applies, so the prompt fires only for genuinely /// wallet-derived keys. /// - /// Returns `Ok(None)` when the key is absent. + /// Note this fixes *placement*, not *derivation*: an + /// `AtWalletDerivationPath` entry can be correctly matched here and still + /// carry a stale path, which is what [`Self::sign_via_hash160_path_scan`] + /// exists to recover from. + /// + /// `Ok(None)` when no placement holds this key. When every candidate failed, + /// the first failure is returned rather than `None`, so a lone dead entry + /// still surfaces its own typed error instead of a silent miss. /// /// [`PrivateKeyData::AtWalletDerivationPath`]: encrypted_key_storage::PrivateKeyData::AtWalletDerivationPath + /// [`PrivateKeyData::InVault`]: encrypted_key_storage::PrivateKeyData::InVault /// [`SecretAccess`]: crate::wallet_backend::SecretAccess pub async fn resolve_private_key_bytes( + &self, + key: &IdentityPublicKey, + ) -> Result, TaskError> { + let mut first_failure = None; + + for (target, key_id) in self.private_keys.candidates(key).collect::>() { + match self.resolve_private_key_bytes_at(target, key_id).await { + Ok(Some(resolved)) => return Ok(Some(resolved)), + // This placement holds no usable bytes. Keep looking: another + // store may hold the same key's live material. + Ok(None) => {} + Err(failure) => { + if first_failure.is_none() { + first_failure = Some(failure); + } + } + } + } + + match first_failure { + Some(failure) => Err(failure), + None => Ok(None), + } + } + + /// Resolve the private key filed at exactly `(target, key_id)`. + /// + /// The single-placement step of [`Self::resolve_private_key_bytes`], which + /// owns the decision of *which* placements to try. Private so no caller can + /// name a placement itself and reintroduce the map-key/vault-label + /// disagreement this design removes. + async fn resolve_private_key_bytes_at( &self, target: PrivateKeyTarget, key_id: KeyID, @@ -1103,6 +1218,573 @@ impl QualifiedIdentity { } } +/// Where a key's private half is filed, and where a new one belongs. +/// +/// The shape under test throughout is a `Purpose::VOTING` key on the **main** +/// identity: real (`masternode_key_presence` reads it as voting readiness on its +/// own) and the one shape no purpose-derived answer can place, since deriving +/// from the purpose sends every voting key to the voter identity. +#[cfg(test)] +mod key_placement_tests { + use super::*; + use crate::model::qualified_identity::encrypted_key_storage::PrivateKeyData; + use crate::model::qualified_identity::key_placement::{KeyPlacement, PROBE_ORDER}; + use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dash_sdk::dpp::version::PlatformVersion; + use dash_sdk::platform::Identifier; + + const MAIN: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnMainIdentity; + const VOTER: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnVoterIdentity; + const OPERATOR: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnOperatorIdentity; + + /// A key whose `data()` is derived from `material`, so two keys sharing an + /// id can still be told apart the way the resolver tells them apart. + fn key(id: KeyID, purpose: Purpose, material: u8) -> IdentityPublicKey { + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_HASH160, + read_only: false, + data: BinaryData::new(vec![material; 20]), + disabled_at: None, + }) + } + + /// An identity publishing `published`, holding the private halves in `held`. + fn qi( + published: &[IdentityPublicKey], + voter: Option<&[IdentityPublicKey]>, + held: &[(PrivateKeyTarget, IdentityPublicKey, PrivateKeyData)], + ) -> QualifiedIdentity { + let pv = PlatformVersion::latest(); + let build = |keys: &[IdentityPublicKey], id: u8| { + Identity::new_with_id_and_keys( + Identifier::from([id; 32]), + keys.iter().map(|k| (k.id(), k.clone())).collect(), + pv, + ) + .expect("identity") + }; + + let mut private_keys = KeyStorage::default(); + for (target, key, data) in held { + private_keys.insert_at( + (target.clone(), key.id()), + (QualifiedIdentityPublicKey::from(key.clone()), data.clone()), + ); + } + + QualifiedIdentity { + identity: build(published, 1), + associated_voter_identity: voter.map(|keys| { + let voter_identity = build(keys, 2); + let first = keys.first().expect("a voter list has a key").clone(); + (voter_identity, first) + }), + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type: IdentityType::Masternode, + alias: None, + private_keys, + dpns_names: vec![], + associated_wallets: BTreeMap::new(), + secret_access: None, + wallet_index: None, + top_ups: BTreeMap::new(), + status: IdentityStatus::Active, + network: Network::Testnet, + } + } + + fn clear(bytes: u8) -> PrivateKeyData { + PrivateKeyData::Clear([bytes; 32]) + } + + /// T1 — the defect. A `VOTING` key on the main identity, its private half + /// filed under `Main`, which is where the authoritative loader + /// (`load_identity`) puts a main-identity key. Deriving the store from the + /// purpose looks under `Voter` and misses it, so the key cannot sign. The + /// resolver must find it. + #[test] + fn voting_key_on_the_main_identity_is_found_where_the_loader_files_it() { + let voting = key(3, Purpose::VOTING, 0xAA); + let identity = qi( + std::slice::from_ref(&voting), + None, + &[(MAIN, voting.clone(), clear(0x11))], + ); + + assert_eq!( + identity + .private_keys + .candidates(&voting) + .collect::>(), + vec![(MAIN, 3)], + "a main-identity voting key's private half is filed under Main" + ); + } + + /// Two keys can share both `id` and `data` — a main identity's voting key + /// and a linked voter identity's key — leaving `purpose` as the only thing + /// telling them apart. Matching on material alone would report one as held + /// on the strength of the other's private half, and a delete aimed at one + /// would take the other with it. + #[test] + fn two_keys_sharing_id_and_material_are_told_apart_by_purpose() { + let voting = key(0, Purpose::VOTING, 0xAA); + let auth = key(0, Purpose::AUTHENTICATION, 0xAA); + assert_eq!(voting.data(), auth.data(), "the fixture shares material"); + + let identity = qi( + std::slice::from_ref(&voting), + None, + &[(VOTER, voting.clone(), clear(0x11))], + ); + + assert_eq!( + identity + .private_keys + .candidates(&voting) + .collect::>(), + vec![(VOTER, 0)], + ); + assert!( + identity.private_keys.candidates(&auth).next().is_none(), + "an occupied slot proves nothing about whose material is in it", + ); + } + + /// `disabled_at` is the one field Platform lets move after a key is added. + /// The stored copy is a snapshot from when the private half was saved, so a + /// key disabled on chain since then must still match — disabling a key does + /// not remove its private half from this device. + #[test] + fn a_key_disabled_since_it_was_saved_is_still_found() { + let active = key(1, Purpose::AUTHENTICATION, 0xBB); + let disabled = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: 1, + purpose: Purpose::AUTHENTICATION, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_HASH160, + read_only: false, + data: BinaryData::new(vec![0xBB; 20]), + disabled_at: Some(1), + }); + + // Stored while active; the live key is the disabled one. + let identity = qi( + std::slice::from_ref(&disabled), + None, + &[(MAIN, active, clear(0x22))], + ); + + assert_eq!( + identity + .private_keys + .candidates(&disabled) + .collect::>(), + vec![(MAIN, 1)], + "a key this device holds stays held after being disabled on chain", + ); + } + + /// T2 — the migration constraint. The same key filed under `Voter` by an + /// older build must stay findable. This is what makes the change safe to + /// ship without moving anyone's key material. + #[test] + fn a_voting_key_an_older_build_filed_under_voter_stays_findable() { + let voting = key(3, Purpose::VOTING, 0xAA); + let identity = qi( + std::slice::from_ref(&voting), + None, + &[(VOTER, voting.clone(), clear(0x11))], + ); + + assert_eq!( + identity + .private_keys + .candidates(&voting) + .collect::>(), + vec![(VOTER, 3)], + "material filed under the legacy convention is still reachable" + ); + } + + /// T3 — the mirror defect. A non-voting key on a voter identity: deriving + /// from the purpose looks under `Main` and misses it. + #[test] + fn an_authentication_key_on_the_voter_identity_is_found() { + let auth = key(0, Purpose::AUTHENTICATION, 0xBB); + let identity = qi( + &[], + Some(std::slice::from_ref(&auth)), + &[(VOTER, auth.clone(), clear(0x22))], + ); + + assert_eq!( + identity.private_keys.candidates(&auth).collect::>(), + vec![(VOTER, 0)], + ); + } + + /// T4 — the collision. The voter and main id spaces overlap, so id 0 can + /// name two different keys. Matching on the id alone picks whichever the + /// derived target names, which is how a delete lands on the wrong key. + /// Candidates must select on material, returning only the requested key. + #[test] + fn two_different_keys_sharing_an_id_are_never_confused() { + let on_main = key(0, Purpose::AUTHENTICATION, 0xAA); + let on_voter = key(0, Purpose::VOTING, 0xBB); + let identity = qi( + std::slice::from_ref(&on_main), + Some(std::slice::from_ref(&on_voter)), + &[ + (MAIN, on_main.clone(), clear(0x11)), + (VOTER, on_voter.clone(), clear(0x22)), + ], + ); + + assert_eq!( + identity + .private_keys + .candidates(&on_main) + .collect::>(), + vec![(MAIN, 0)], + "the main-identity key resolves only to its own entry" + ); + assert_eq!( + identity + .private_keys + .candidates(&on_voter) + .collect::>(), + vec![(VOTER, 0)], + "the voter-identity key resolves only to its own entry" + ); + } + + /// T6 — determinism. The same material filed under two stores yields both + /// candidates in [`PROBE_ORDER`], never in whatever order the map iterates. + #[test] + fn duplicate_placements_are_returned_in_probe_order() { + let voting = key(1, Purpose::VOTING, 0xAA); + let identity = qi( + std::slice::from_ref(&voting), + None, + &[ + (VOTER, voting.clone(), clear(0x11)), + (MAIN, voting.clone(), clear(0x11)), + (OPERATOR, voting.clone(), clear(0x11)), + ], + ); + + assert_eq!( + identity + .private_keys + .candidates(&voting) + .collect::>(), + vec![(MAIN, 1), (VOTER, 1), (OPERATOR, 1)], + "candidates follow the fixed probe order" + ); + assert_eq!( + PROBE_ORDER, + [MAIN, VOTER, OPERATOR], + "probe order is the documented one" + ); + } + + /// T7 — the load-bearing assumption. Every writer copies the on-chain key + /// into the entry, so the stored public half is the same record the caller + /// later presents. An entry whose material disagrees is not this key, and + /// must not be offered as a candidate — that is what keeps the material + /// match from resolving someone else's secret. + #[test] + fn an_entry_whose_material_disagrees_is_not_a_candidate() { + let requested = key(2, Purpose::AUTHENTICATION, 0xAA); + let impostor = key(2, Purpose::AUTHENTICATION, 0xCC); + let identity = qi( + std::slice::from_ref(&requested), + None, + &[(MAIN, impostor, clear(0x33))], + ); + + assert!( + identity + .private_keys + .candidates(&requested) + .next() + .is_none(), + "a same-id entry holding different material is not this key" + ); + } + + /// T9 — legacy reach. Nothing in this app writes + /// `PrivateKeyOnOperatorIdentity` any more, but a v0.9.3-era blob can carry + /// one. Dropping it from the probe order would strand exactly those keys. + #[test] + fn an_operator_filed_key_from_a_legacy_blob_stays_reachable() { + let owner = key(0, Purpose::OWNER, 0xDD); + let identity = qi( + std::slice::from_ref(&owner), + None, + &[(OPERATOR, owner.clone(), clear(0x44))], + ); + + assert_eq!( + identity.private_keys.candidates(&owner).collect::>(), + vec![(OPERATOR, 0)], + ); + } + + /// A key with no private half held yields no candidates — absence is not an + /// error, and must not fall back to some other key. + #[test] + fn a_key_whose_private_half_is_not_held_yields_nothing() { + let published = key(0, Purpose::AUTHENTICATION, 0xAA); + let identity = qi(std::slice::from_ref(&published), None, &[]); + + assert!( + identity + .private_keys + .candidates(&published) + .next() + .is_none() + ); + } + + /// `placement_of` answers the *other* question: where a new private half + /// belongs. For a voting key published on the main identity that is `Main`, + /// which is exactly where the purpose derivation would not have put it. + #[test] + fn placement_of_reads_the_identitys_own_lists() { + let on_main = key(3, Purpose::VOTING, 0xAA); + let on_voter = key(0, Purpose::VOTING, 0xBB); + let identity = qi( + std::slice::from_ref(&on_main), + Some(std::slice::from_ref(&on_voter)), + &[], + ); + + assert_eq!( + identity.placement_of(&on_main), + KeyPlacement::Resolved(MAIN), + "a voting key published on the main identity belongs to Main" + ); + assert_eq!( + identity.placement_of(&on_voter), + KeyPlacement::Resolved(VOTER), + ); + } + + /// A key on no list is `Unknown`, not a guess. Normal for a locally-added + /// key whose state transition has not been broadcast — `add_key_to_identity` + /// inserts before it builds the transition, so this is the steady state + /// there, and asserting a placement at that moment would fire on every add. + #[test] + fn a_key_on_no_list_is_unknown_rather_than_defaulted() { + let published = key(0, Purpose::AUTHENTICATION, 0xAA); + let fresh = key(7, Purpose::AUTHENTICATION, 0xEE); + let identity = qi(&[published], None, &[]); + + assert_eq!(identity.placement_of(&fresh), KeyPlacement::Unknown); + assert_eq!(identity.placement_of(&fresh).resolved(), None); + } + + /// The same key published on two lists is `Ambiguous` — reported, never + /// silently collapsed to one of them. + #[test] + fn a_key_published_on_two_lists_is_ambiguous() { + let shared = key(0, Purpose::VOTING, 0xAA); + let identity = qi( + std::slice::from_ref(&shared), + Some(std::slice::from_ref(&shared)), + &[], + ); + + assert_eq!( + identity.placement_of(&shared), + KeyPlacement::Ambiguous(vec![MAIN, VOTER]), + ); + assert_eq!(identity.placement_of(&shared).resolved(), None); + } +} + +/// Whether a key that is held can actually be used. +/// +/// The regression lock for the failure this change exists to prevent: a key the +/// app accepted and saved, that no signing path could ever find, because the +/// writer and the reader disagreed about which store it went into. Both +/// placements are covered, so the two can never drift apart again in either +/// direction. +#[cfg(test)] +mod key_resolution_tests { + use super::*; + use crate::model::qualified_identity::encrypted_key_storage::PrivateKeyData; + use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dash_sdk::dpp::version::PlatformVersion; + use dash_sdk::platform::Identifier; + + const MAIN: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnMainIdentity; + const VOTER: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnVoterIdentity; + + fn voting_key(id: KeyID) -> IdentityPublicKey { + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose: Purpose::VOTING, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_HASH160, + read_only: false, + data: BinaryData::new(vec![0xAA; 20]), + disabled_at: None, + }) + } + + /// A masternode publishing `key` on its MAIN identity, with a private-half + /// entry per `placements` — enough to build the divergent and duplicate + /// shapes a real install can contain. + fn masternode_with( + key: &IdentityPublicKey, + placements: &[(PrivateKeyTarget, PrivateKeyData)], + ) -> QualifiedIdentity { + let pv = PlatformVersion::latest(); + let identity = Identity::new_with_id_and_keys( + Identifier::from([1u8; 32]), + BTreeMap::from([(key.id(), key.clone())]), + pv, + ) + .expect("identity"); + + let mut private_keys = KeyStorage::default(); + for (target, data) in placements { + private_keys.insert_at( + (target.clone(), key.id()), + (QualifiedIdentityPublicKey::from(key.clone()), data.clone()), + ); + } + + QualifiedIdentity { + identity, + associated_voter_identity: None, + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type: IdentityType::Masternode, + alias: None, + private_keys, + dpns_names: vec![], + associated_wallets: BTreeMap::new(), + secret_access: None, + wallet_index: None, + top_ups: BTreeMap::new(), + status: IdentityStatus::Active, + network: Network::Testnet, + } + } + + /// T0 — a `Purpose::VOTING` key on the main identity must be signable + /// wherever its private half is filed. Held under `Main` is what the + /// authoritative loader writes and what the structural target names; held + /// under `Voter` is what older builds wrote. A signing path that only looks + /// in one of the two reports a saved key as unusable. + #[tokio::test] + async fn a_held_voting_key_on_the_main_identity_is_signable_under_either_placement() { + let key = voting_key(3); + let secret = [0x11; 32]; + + for filed_under in [MAIN, VOTER] { + let identity = masternode_with( + &key, + &[(filed_under.clone(), PrivateKeyData::Clear(secret))], + ); + + assert!( + identity.can_sign_with(&key), + "a held voting key filed under {filed_under:?} must report as signable", + ); + + let (_, resolved) = identity + .resolve_private_key_bytes(&key) + .await + .expect("resolution must not fail") + .unwrap_or_else(|| { + panic!("a held voting key filed under {filed_under:?} must yield its bytes") + }); + assert_eq!( + *resolved, secret, + "the bytes resolved are the ones filed under {filed_under:?}", + ); + } + } + + /// T5 — the fallthrough rule. A vault placeholder whose secret is gone can + /// sit beside a live entry for the same key under another store. A resolver + /// that stopped at the first *matching* placement would report the key + /// unusable with its bytes one probe away; it must return the first + /// placement that actually yields bytes. + #[tokio::test] + async fn a_dead_vault_placeholder_falls_through_to_a_live_placement() { + let key = voting_key(0); + let secret = [0x77; 32]; + // Main is probed first and holds an InVault placeholder with no + // chokepoint wired, so it cannot produce bytes. + let identity = masternode_with( + &key, + &[ + (MAIN, PrivateKeyData::InVault), + (VOTER, PrivateKeyData::Clear(secret)), + ], + ); + + let (_, resolved) = identity + .resolve_private_key_bytes(&key) + .await + .expect("a live placement exists, so resolution must not fail") + .expect("the live placement must be found"); + assert_eq!( + *resolved, secret, + "resolution falls through the dead placeholder to the live bytes", + ); + } + + /// The other half of the fallthrough rule: with nothing to fall through to, + /// a dead placement keeps surfacing its own typed error. Degrading it to + /// `Ok(None)` would turn a recoverable "your key is in the vault and the + /// vault is not open" into a silent "you never had that key". + #[tokio::test] + async fn a_lone_dead_placement_surfaces_its_error_rather_than_absence() { + let key = voting_key(0); + let identity = masternode_with(&key, &[(MAIN, PrivateKeyData::InVault)]); + + let error = identity + .resolve_private_key_bytes(&key) + .await + .expect_err("a vault-backed key with no chokepoint cannot resolve"); + assert!( + matches!(error, TaskError::WalletLocked), + "expected the vault-unavailable error, got {error:?}", + ); + } + + /// A key this identity holds no private half for resolves to `None` — an + /// absence, not an error, and never another key's material. + #[tokio::test] + async fn a_key_with_no_placement_resolves_to_absence() { + let key = voting_key(0); + let identity = masternode_with(&key, &[]); + + assert!( + identity + .resolve_private_key_bytes(&key) + .await + .expect("absence is not a failure") + .is_none() + ); + assert!(!identity.can_sign_with(&key)); + } +} + #[cfg(test)] mod masternode_key_presence_tests { use super::*; @@ -1139,7 +1821,7 @@ mod masternode_key_presence_tests { let mut ks = KeyStorage::default(); for (i, purpose) in main_key_purposes.iter().enumerate() { let key = key_with_purpose(i as KeyID, *purpose); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, key.id()), ( QualifiedIdentityPublicKey::from(key), @@ -1270,7 +1952,7 @@ mod withdrawal_key_tests { associated_owner_key_id: None, identity_type, alias: None, - private_keys: KeyStorage { private_keys }, + private_keys: KeyStorage::from(private_keys), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, diff --git a/src/ui/identities/keys/key_info_screen.rs b/src/ui/identities/keys/key_info_screen.rs index 9862f3a11..e36230fb8 100644 --- a/src/ui/identities/keys/key_info_screen.rs +++ b/src/ui/identities/keys/key_info_screen.rs @@ -114,9 +114,6 @@ pub struct KeyInfoScreen { recovery: LegacyRecoveryState, /// A queued restore (the approved items), drained in `ui()`. pending_recovery_restore: Option>, - /// Which of the identity's key stores this key is filed under, when the - /// caller knew. See [`Self::target`] for why that beats deriving it. - target: Option, /// The screen this key was opened from, for the breadcrumb back to it. parent: Option<&'static str>, } @@ -300,7 +297,7 @@ impl ScreenLike for KeyInfoScreen { ui.label(RichText::new("Purpose:").strong().color(text_primary)); let (role, role_tip) = key_role_label( KeyVocabulary::from(self.identity.identity_type), - &self.target(), + &self.naming_target(), &self.key, ); let purpose_label = ui.label(RichText::new(role).color(text_primary)); @@ -760,27 +757,43 @@ impl ScreenLike for KeyInfoScreen { // Vault-backed (InVault) identity-key requests: the raw key is fetched // JIT in the backend and only the public WIF / signature returns. let identity_id = self.identity.identity.id(); - let target: PrivateKeyTarget = self.target(); let key_id = self.key.id(); - if std::mem::take(&mut self.pending_identity_key_display) { - action |= AppAction::BackendTask(BackendTask::WalletTask( - WalletTask::DeriveIdentityKeyForDisplay { - identity_id, - target: target.clone(), - key_id, - }, - )); - } - if std::mem::take(&mut self.pending_identity_sign) { - action |= AppAction::BackendTask(BackendTask::WalletTask( - WalletTask::SignMessageWithIdentityKey { - identity_id, - target, - key_id, - message: self.message_input.clone(), - key_type: self.key.key_type(), - }, - )); + let wants_display = std::mem::take(&mut self.pending_identity_key_display); + let wants_sign = std::mem::take(&mut self.pending_identity_sign); + if wants_display || wants_sign { + // The vault stores each key under the store it is filed in, so the + // request has to name the placement the material is actually at. + match self.target() { + Some(target) => { + if wants_display { + action |= AppAction::BackendTask(BackendTask::WalletTask( + WalletTask::DeriveIdentityKeyForDisplay { + identity_id, + target: target.clone(), + key_id, + }, + )); + } + if wants_sign { + action |= AppAction::BackendTask(BackendTask::WalletTask( + WalletTask::SignMessageWithIdentityKey { + identity_id, + target, + key_id, + message: self.message_input.clone(), + key_type: self.key.key_type(), + }, + )); + } + } + None => { + MessageBanner::set_global( + ctx, + "This key is not saved on this device, so it cannot be shown or used to sign.", + MessageType::Error, + ); + } + } } // Drain a queued identity-key protection opt-in / opt-out. @@ -882,11 +895,26 @@ impl KeyInfoScreen { pending_unprotect: None, recovery, pending_recovery_restore: None, - target: None, parent: None, } } + /// The store this key is *published* under, for naming it. + /// + /// Deliberately the structural answer rather than [`Self::target`]'s: a key's + /// name follows the identity list it belongs to — which is what + /// `identity_keys` pairs it with on every list that shows it — not wherever + /// its private half happens to be filed. Naming it from the material's + /// location would let one key be called two things depending on which build + /// saved it. `Unknown` names the main identity, which is where a key not yet + /// on any list is being added. + fn naming_target(&self) -> PrivateKeyTarget { + self.identity + .placement_of(&self.key) + .resolved() + .unwrap_or(PrivateKeyTarget::PrivateKeyOnMainIdentity) + } + /// Name the screen this key was opened from, so the breadcrumb can lead back /// to it. /// @@ -915,39 +943,23 @@ impl KeyInfoScreen { } } - /// Record which key store this key is filed under, for a caller that already - /// resolved it from the identity the key was listed from. + /// The key store this screen's key is filed under. /// - /// Prefer this wherever the target is known. Without it the screen falls - /// back to deriving the target from the key's purpose, which cannot - /// distinguish a voting key filed on the main identity from one on a voter - /// identity — see [`Self::target`]. - pub fn with_target(mut self, target: PrivateKeyTarget) -> Self { - self.target = Some(target); - self - } - - /// The key store this key is filed under, for **reads**: what the caller - /// resolved, else derived from the key's purpose. + /// Resolved from the identity's own records every time it is asked, so no + /// caller has to supply it and none can supply a wrong one. An existing + /// placement wins — that is where the material actually is — and only when + /// nothing is held does the identity's on-chain lists decide where a new + /// private half would go. /// - /// Reads only, and deliberately. Looking somewhere else can only find - /// material that is already there, so it is safe today; *writing* somewhere - /// else would put material where `QualifiedIdentity::sign` never looks. The - /// write and remove paths therefore keep the derived target until every - /// reader is migrated together. - /// - /// The derivation is lossy and cannot be made otherwise: - /// `impl From for PrivateKeyTarget` sends every `Purpose::VOTING` - /// key to the voter identity, but a voting-purpose key filed on the main - /// identity is a supported shape (`masternode_key_presence` reads it as - /// voting readiness on its own). For that key the derivation names a store - /// it was never filed under, so a read misses it and a write or delete - /// lands on a different key that happens to share its id. Only the caller - /// that walked the identity knows which store it came from. - fn target(&self) -> PrivateKeyTarget { - self.target - .clone() - .unwrap_or_else(|| self.key.purpose().into()) + /// `None` when the key is on none of this identity's lists and nothing is + /// held for it, which is the one case where no store can be named honestly. + fn target(&self) -> Option { + self.identity + .private_keys + .candidates(&self.key) + .next() + .map(|(target, _)| target) + .or_else(|| self.identity.placement_of(&self.key).resolved()) } /// Re-read this screen's identity from the store, after a backend task @@ -963,9 +975,19 @@ impl KeyInfoScreen { let identity_id = self.identity.identity.id(); match self.app_context.get_local_qualified_identity(&identity_id) { Ok(Some(fresh)) => { - self.private_key_data = fresh - .private_keys - .get_cloned_private_key_data_and_wallet_info(&(self.target(), self.key.id())); + // Resolved against the record just read, not the stale clone: + // the write being picked up here may be the one that filed this + // key in the first place. + self.private_key_data = + fresh + .private_keys + .candidates(&self.key) + .next() + .and_then(|placement| { + fresh + .private_keys + .get_cloned_private_key_data_and_wallet_info(&placement) + }); self.identity = fresh; } Ok(None) => {} @@ -1065,13 +1087,20 @@ impl KeyInfoScreen { } else if validation_result.expect("invariant: Err handled in the preceding branch") { // If valid, store the private key in the context and reset the input field self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None)); - // Deliberately the purpose-derived target, not `self.target()`: - // `QualifiedIdentity::sign` and `can_sign_with` look the key up that - // way, so material stored anywhere else is material that can never - // sign. Writing the resolved target instead needs every reader - // migrated with it — see the reconciliation TODO. + // An existing placement is reused so a re-entered key overwrites + // itself rather than growing a second copy under another store; + // otherwise the identity's own lists say where it belongs. Both + // agree with where the resolver will look for it. + let Some(target) = self.target() else { + MessageBanner::set_global( + self.app_context.egui_ctx(), + "This key does not belong to this identity, so it cannot be saved here.", + MessageType::Error, + ); + return; + }; self.identity.private_keys.insert_non_encrypted( - (self.key.purpose().into(), self.key.id()), + (target, self.key.id()), (self.key.clone().into(), private_key_bytes), ); if let Err(error) = self @@ -1270,35 +1299,46 @@ impl KeyInfoScreen { let response = dialog.show(ui); if let Some(result) = response.inner.dialog_response { self.remove_private_key_dialog = None; - if result == ConfirmationStatus::Confirmed { - self.private_key_data = None; - // The purpose-derived target for the same reason as the - // write above: this has to remove the entry the rest of the - // app would have used. That it can therefore remove a - // different key's material, when a voter identity carries - // the same key id, is the known residual of the split - // conventions — the reconciliation TODO owns it. - self.identity - .private_keys - .private_keys - .remove(&(self.key.purpose().into(), self.key.id())); - if let Err(error) = self - .app_context - .update_local_qualified_identity(&self.identity) - { - let handle = MessageBanner::set_global( - ui.ctx(), - "The private-key change could not be saved. Check available disk space and try again.", - MessageType::Error, - ); - handle.with_details(error); - handle.disable_auto_dismiss(); - } + if result == ConfirmationStatus::Confirmed + && let Err(error) = self.remove_held_private_key() + { + let handle = MessageBanner::set_global( + ui.ctx(), + "The private-key change could not be saved. Check available disk space and try again.", + MessageType::Error, + ); + handle.with_details(error); + handle.disable_auto_dismiss(); } } } } + /// Drop this device's copy of the on-screen key's private half and persist + /// the record. + /// + /// Removes **every** placement holding *this* key, so a duplicate written + /// under another convention cannot survive the removal the user asked for. + /// The placements come from + /// [`candidates`](crate::model::qualified_identity::encrypted_key_storage::KeyStorage::candidates), + /// which selects on the public half — a removal keyed on the id alone lands + /// on whichever key happens to occupy the derived slot, and on a masternode + /// the voter and main id spaces overlap, so that can be a different key + /// entirely. + fn remove_held_private_key(&mut self) -> Result<(), TaskError> { + self.private_key_data = None; + for placement in self + .identity + .private_keys + .candidates(&self.key) + .collect::>() + { + self.identity.private_keys.remove_at(&placement); + } + self.app_context + .update_local_qualified_identity(&self.identity) + } + // --- Identity key password protection (per-identity at-rest key encryption) --- /// At-rest protection posture of this identity's vault keys, by probing the @@ -1643,6 +1683,7 @@ mod tests { use std::collections::BTreeMap; const MAIN: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnMainIdentity; + const VOTER: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnVoterIdentity; /// An offline, wired context on a throwaway data dir — the identity store /// refuses writes until the wallet backend is up. @@ -1689,7 +1730,7 @@ mod tests { fn identity_with(id: u8, keys: &[(IdentityPublicKey, [u8; 32])]) -> QualifiedIdentity { let mut private_keys = KeyStorage::default(); for (key, secret) in keys { - private_keys.private_keys.insert( + private_keys.insert_at( (MAIN, key.id()), ( QualifiedIdentityPublicKey::from(key.clone()), @@ -1719,6 +1760,72 @@ mod tests { } } + /// Removing this device's copy of one key must not touch a *different* key + /// that happens to share its id. + /// + /// The shape this reaches needs two writers, which is how a real install gets + /// it: the structural loader files a main-identity `VOTING` key under `Main` + /// (`load_identity` files every main-identity key structurally, purpose + /// included), while an older build's paste path filed an unrelated key under + /// `Voter` at the same id — the voter and main id spaces overlap, so id 0 + /// names two keys on a masternode. A removal that derived its slot from the + /// key's purpose would send this one to `Voter` and delete the wrong key's + /// private half, leaving the key the user asked about still on the device. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn removing_one_key_leaves_a_different_key_sharing_its_id_alone() { + let (app_context, _dir) = offline_ctx().await; + + // Same id, different keys: purpose is what tells them apart. + let on_screen = public_key(0, Purpose::VOTING); + let other = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: 0, + purpose: Purpose::AUTHENTICATION, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_HASH160, + read_only: false, + data: BinaryData::new(vec![0xEE; 20]), + disabled_at: None, + }); + + let mut stored = identity_with(0x5A, &[(on_screen.clone(), [0x11; 32])]); + stored.private_keys.insert_at( + (VOTER, other.id()), + ( + QualifiedIdentityPublicKey::from(other.clone()), + PrivateKeyData::Clear([0x22; 32]), + ), + ); + app_context + .insert_local_qualified_identity(&stored, &None) + .expect("insert the record"); + + let mut screen = + KeyInfoScreen::new(stored, on_screen.clone(), None, &app_context).with_parent("Keys"); + screen + .remove_held_private_key() + .expect("the removal must persist"); + + assert!( + screen + .identity + .private_keys + .candidates(&on_screen) + .next() + .is_none(), + "the key the user asked to remove must be gone", + ); + assert!( + screen + .identity + .private_keys + .candidates(&other) + .next() + .is_some(), + "a different key sharing the id must survive the removal", + ); + } + /// Write `key` into `identity_id`'s stored record, the way a restore or any /// other backend writer does — behind whatever screen holds a clone of it. fn write_key_behind_the_screen( @@ -1731,7 +1838,7 @@ mod tests { .get_local_qualified_identity(&identity_id) .expect("read the record") .expect("record stored"); - record.private_keys.private_keys.insert( + record.private_keys.insert_at( (MAIN, key.id()), ( QualifiedIdentityPublicKey::from(key.clone()), @@ -1773,11 +1880,7 @@ mod tests { }); assert!( - screen - .identity - .private_keys - .private_keys - .contains_key(&(MAIN, restored_key.id())), + screen.identity.private_keys.has(&(MAIN, restored_key.id())), "the screen must hold the restored record, not the clone it opened with", ); @@ -1788,6 +1891,52 @@ mod tests { .await; } + /// A restore that lands while this screen is off-screen never reaches its + /// `display_task_result` — results go only to the visible screen. Returning + /// to it must re-read the record, or the clone it opened with is written + /// back over the restored keys by the next ordinary key edit, silently and + /// with no error to show for it. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_restore_that_landed_off_screen_survives_the_next_key_edit() { + let (app_context, _dir) = offline_ctx().await; + + let on_screen_key = public_key(1, Purpose::AUTHENTICATION); + let stored = identity_with(0x4E, &[(on_screen_key.clone(), [0x11; 32])]); + let identity_id = stored.identity.id(); + app_context + .insert_local_qualified_identity(&stored, &None) + .expect("insert the record"); + let mut screen = KeyInfoScreen::new(stored, on_screen_key, None, &app_context); + + // The restore lands while another screen is the visible one, so this + // screen is never told about it. + let restored_key = public_key(2, Purpose::TRANSFER); + write_key_behind_the_screen(&app_context, identity_id, &restored_key, [0x22; 32]); + + screen.refresh_on_arrival(); + + // What every key add and remove on this screen does with its clone. + app_context + .update_local_qualified_identity(&screen.identity) + .expect("the next key edit's write"); + + assert!( + app_context + .get_local_qualified_identity(&identity_id) + .expect("read back") + .expect("still stored") + .private_keys + .has(&(MAIN, restored_key.id())), + "a key edit on this screen must not erase keys restored while it was away", + ); + + app_context + .wallet_backend() + .expect("backend") + .shutdown() + .await; + } + /// A restore dispatched from one identity's Key Info screen can complete /// after the user has opened another's, and results reach whichever screen /// is visible. The stray completion must touch nothing here: not the clone, @@ -1831,8 +1980,7 @@ mod tests { !screen .identity .private_keys - .private_keys - .contains_key(&(MAIN, other_writer_key.id())), + .has(&(MAIN, other_writer_key.id())), "another identity's completion must not be acted on here at all", ); diff --git a/src/ui/identities/keys/keys_screen.rs b/src/ui/identities/keys/keys_screen.rs index e3c3c2104..cf99978d1 100644 --- a/src/ui/identities/keys/keys_screen.rs +++ b/src/ui/identities/keys/keys_screen.rs @@ -19,7 +19,7 @@ use crate::ui::components::legacy_recovery_section::host_offer; use crate::ui::components::styled::island_central_panel; use crate::ui::components::top_panel::add_top_panel; use crate::ui::identities::keys::key_info_screen::KeyInfoScreen; -use crate::ui::masternodes::{KeyVocabulary, identity_keys, key_filed_at, manage_keys_labels}; +use crate::ui::masternodes::{KeyVocabulary, identity_keys, manage_keys_labels}; use crate::ui::state::legacy_recovery::LegacyRecoveryState; use crate::ui::theme::{ComponentStyles, DashColors, ResponseExt}; use crate::ui::{MessageType, RootScreenType, Screen, ScreenLike}; @@ -181,8 +181,12 @@ impl KeysScreen { let expert = self.app_context.user_role().at_least(UserRole::Power); let vocabulary = KeyVocabulary::from(self.identity.identity_type); let labels = manage_keys_labels(vocabulary, &keys); - for ((target, key), (label, tip)) in keys.into_iter().zip(labels) { - let filed_at = key_filed_at(&self.identity, &target, &key); + for ((_, key), (label, tip)) in keys.into_iter().zip(labels) { + // Where this key's private half actually is, whichever store filed + // it. A presence check rather than a fetch: cloning the entry copies + // raw key bytes out of the vault unscrubbed, and this runs every + // frame for every key. + let filed_at = self.identity.private_keys.candidates(&key).next(); let held = if filed_at.is_some() { HELD } else { NOT_HELD }; ui.add_space(4.0); ui.horizontal(|ui| { @@ -193,14 +197,11 @@ impl KeysScreen { None => button, }; if button.clicked() { - let opened_at = filed_at.clone().unwrap_or_else(|| target.clone()); - let holding = self - .identity - .private_keys - .get_cloned_private_key_data_and_wallet_info(&( - opened_at.clone(), - key.id(), - )); + let holding = filed_at.as_ref().and_then(|placement| { + self.identity + .private_keys + .get_cloned_private_key_data_and_wallet_info(placement) + }); action |= AppAction::AddScreen(Screen::KeyInfoScreen( KeyInfoScreen::new( self.identity.clone(), @@ -208,9 +209,6 @@ impl KeysScreen { holding, &self.app_context, ) - // Where the material actually is, so the screen's own - // re-read finds the same key this row just reported. - .with_target(opened_at) .with_parent(PARENT_CRUMB), )); } diff --git a/src/ui/identities/mod.rs b/src/ui/identities/mod.rs index d582aa176..d49ac9adf 100644 --- a/src/ui/identities/mod.rs +++ b/src/ui/identities/mod.rs @@ -1,19 +1,13 @@ use std::sync::{Arc, RwLock}; use dash_sdk::{ - dpp::{ - data_contract::accessors::v0::DataContractV0Getters, - identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0, - }, - platform::IdentityPublicKey, + dpp::data_contract::accessors::v0::DataContractV0Getters, platform::IdentityPublicKey, }; use crate::{ context::AppContext, model::{ - qualified_identity::{ - PrivateKeyTarget, QualifiedIdentity, encrypted_key_storage::PrivateKeyData, - }, + qualified_identity::{QualifiedIdentity, encrypted_key_storage::PrivateKeyData}, wallet::Wallet, }, }; @@ -87,13 +81,13 @@ pub fn get_selected_wallet( }; // Once we have the public key (either from DPNS or directly), look up - // the matching private key data in `qualified_identity`. - let key_lookup = (PrivateKeyTarget::PrivateKeyOnMainIdentity, public_key.id()); + // the matching private key data wherever it is filed. + let filed_at = qualified_identity + .private_keys + .candidates(public_key) + .next(); if let Some((_, PrivateKeyData::AtWalletDerivationPath(wallet_derivation_path))) = - qualified_identity - .private_keys - .private_keys - .get(&key_lookup) + filed_at.and_then(|placement| qualified_identity.private_keys.entry_at(&placement)) { // If found, return the associated wallet (cloned to preserve Arc). Ok(qualified_identity diff --git a/src/ui/identities/withdraw_screen.rs b/src/ui/identities/withdraw_screen.rs index f4fdcb8fe..56afb6353 100644 --- a/src/ui/identities/withdraw_screen.rs +++ b/src/ui/identities/withdraw_screen.rs @@ -5,7 +5,7 @@ use crate::context::AppContext; use crate::model::amount::Amount; use crate::model::fee_estimation::{format_credits_as_dash, max_spendable_credits}; use crate::model::qualified_identity::encrypted_key_storage::PrivateKeyData; -use crate::model::qualified_identity::{IdentityType, PrivateKeyTarget, QualifiedIdentity}; +use crate::model::qualified_identity::{IdentityType, QualifiedIdentity}; use crate::model::user_role::UserRole; use crate::model::wallet::Wallet; use crate::ui::components::amount_input::AmountInput; @@ -493,13 +493,13 @@ impl ScreenLike for WithdrawalScreen { // Render wallet unlock component if needed if let Some(selected_key) = self.selected_key.as_ref() { // If there is an associated wallet then render the wallet unlock component for it if its locked + let filed_at = self.identity.private_keys.candidates(selected_key).next(); if let Some(( _, PrivateKeyData::AtWalletDerivationPath(wallet_derivation_path), - )) = self.identity.private_keys.private_keys.get(&( - PrivateKeyTarget::PrivateKeyOnMainIdentity, - selected_key.id(), - )) { + )) = filed_at + .and_then(|placement| self.identity.private_keys.entry_at(&placement)) + { let new_wallet = self .identity .associated_wallets diff --git a/src/ui/masternodes/detail_screen.rs b/src/ui/masternodes/detail_screen.rs index 9672d2ccc..16022110d 100644 --- a/src/ui/masternodes/detail_screen.rs +++ b/src/ui/masternodes/detail_screen.rs @@ -10,6 +10,7 @@ use chrono::{LocalResult, TimeZone, Utc}; use chrono_humanize::HumanTime; use dash_sdk::dpp::identity::TimestampMillis; use dash_sdk::dpp::identity::accessors::IdentityGettersV0; +#[cfg(test)] use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dash_sdk::dpp::platform_value::string_encoding::Encoding; use eframe::egui::{self, Color32, RichText, Ui}; @@ -41,9 +42,7 @@ use crate::ui::identity::identity_pill::shorten_id; use crate::ui::masternodes::card::{ PLATFORM_IDENTITY_STATUS_TOOLTIP, platform_identity_status_label, }; -use crate::ui::masternodes::{ - KeyVocabulary, identity_keys, key_filed_at, key_status_tokens, manage_keys_labels, -}; +use crate::ui::masternodes::{KeyVocabulary, identity_keys, key_status_tokens, manage_keys_labels}; use crate::ui::state::legacy_recovery::LegacyRecoveryState; use crate::ui::theme::{ComponentStyles, DashColors, ResponseExt}; use crate::ui::tokens::claim_tokens_screen::ClaimTokensScreen; @@ -665,14 +664,14 @@ impl MasternodeDetailView { let keys = identity_keys(&self.identity); // This page only ever shows masternode and evonode identities. let labels = manage_keys_labels(KeyVocabulary::from(self.identity.identity_type), &keys); - for ((target, key), (label, tip)) in keys.into_iter().zip(labels) { + for ((_target, key), (label, tip)) in keys.into_iter().zip(labels) { let button = ui.button(format!("{label} ›")); let button = match tip { Some(tip) => button.clickable_tooltip(tip), None => button, }; if button.clicked() { - action = Some(self.open_key_info(target, &key)); + action = Some(self.open_key_info(&key)); } } @@ -681,10 +680,10 @@ impl MasternodeDetailView { // lives inside `KeyInfoScreen`. Open the first held key so the user // lands directly on the interactive seal flow. if tier.offers_add_protection() - && let Some((target, key)) = self.first_protectable_key() + && let Some((_target, key)) = self.first_protectable_key() && ui.button("Add password protection…").clicked() { - action = Some(self.open_key_info_with_protection_prompt(target, &key)); + action = Some(self.open_key_info_with_protection_prompt(&key)); } if let Some(approved) = self.render_recovery_section(ui) @@ -713,55 +712,59 @@ impl MasternodeDetailView { /// The first key whose private material this node actually holds — the only /// keys that can be sealed. Used to route the Add-protection CTA straight /// into an interactive `KeyInfoScreen` seal flow. + /// + /// Resolves "held" through `candidates()`, the same rule every other + /// resolution site on this identity uses — a structural `(target, key_id)` + /// probe would miss material filed under a target other than the one + /// `identity_keys` structurally pairs the key with (e.g. a main-identity + /// voting key filed under the voter placement by an older build), and could + /// match a different key that merely shares the id. `candidates()` only + /// checks presence, so no raw key bytes are cloned out of the vault here — + /// unlike `open_key_info_with_mode`, which needs the actual secret and thus + /// pays for the clone. fn first_protectable_key( &self, ) -> Option<(PrivateKeyTarget, dash_sdk::platform::IdentityPublicKey)> { identity_keys(&self.identity) .into_iter() - .find(|(target, key)| { - self.identity - .private_keys - .get_cloned_private_key_data_and_wallet_info(&(target.clone(), key.id())) - .is_some() - }) + .find(|(_, key)| self.identity.private_keys.candidates(key).next().is_some()) } /// Build the `AddScreen` action that opens `KeyInfoScreen` for one key, /// carrying its held private-key data if any. Mirrors the /// per-key push in `identities_screen.rs`. - fn open_key_info( - &self, - target: PrivateKeyTarget, - key: &dash_sdk::platform::IdentityPublicKey, - ) -> AppAction { - self.open_key_info_with_mode(target, key, KeyInfoOpenMode::Normal) + fn open_key_info(&self, key: &dash_sdk::platform::IdentityPublicKey) -> AppAction { + self.open_key_info_with_mode(key, KeyInfoOpenMode::Normal) } /// Open `KeyInfoScreen` directly in the add-protection confirmation flow. fn open_key_info_with_protection_prompt( &self, - target: PrivateKeyTarget, key: &dash_sdk::platform::IdentityPublicKey, ) -> AppAction { - self.open_key_info_with_mode(target, key, KeyInfoOpenMode::WithProtectionPrompt) + self.open_key_info_with_mode(key, KeyInfoOpenMode::WithProtectionPrompt) } fn open_key_info_with_mode( &self, - target: PrivateKeyTarget, key: &dash_sdk::platform::IdentityPublicKey, mode: KeyInfoOpenMode, ) -> AppAction { // Where this key's private half actually is, by the one rule every - // "Manage keys" surface uses. The structural target alone would miss - // material filed under the purpose-derived convention — a main-identity - // voting key entered by hand — and report a key as unheld here while the - // identity keys list shows it as saved on this device. - let filed_at = key_filed_at(&self.identity, &target, key).unwrap_or(target); + // surface uses. A structural target alone would miss material filed under + // the retired purpose-derived convention — a main-identity voting key + // entered by hand — and report a key as unheld here while the identity + // keys list shows it as saved on this device. let holding = self .identity .private_keys - .get_cloned_private_key_data_and_wallet_info(&(filed_at.clone(), key.id())); + .candidates(key) + .next() + .and_then(|placement| { + self.identity + .private_keys + .get_cloned_private_key_data_and_wallet_info(&placement) + }); let identity = self.identity.clone(); let key = key.clone(); let screen = match mode { @@ -772,12 +775,10 @@ impl MasternodeDetailView { KeyInfoScreen::new_with_protection_prompt(identity, key, holding, &self.app_context) } }; - // Hand the resolved location over rather than letting the screen guess. - // Left to itself it re-derives the target from the key's purpose, which - // disagrees for a key filed on the voter identity whose purpose is not - // `VOTING` — it would name the key differently from the row just clicked - // and lose the private half on its own re-read. - AppAction::AddScreen(Screen::KeyInfoScreen(screen.with_target(filed_at))) + // No target is handed over: the screen resolves the placement itself, so + // there is nothing for this caller to get wrong or for the `ScreenType` + // round trip to drop. + AppAction::AddScreen(Screen::KeyInfoScreen(screen)) } /// Render the collapsible DPNS voting section (collapsed by default, @@ -1188,7 +1189,7 @@ mod tests { let pv = PlatformVersion::latest(); let owner = IdentityPublicKey::random_key(1, Some(1), pv); let mut ks = KeyStorage::default(); - ks.private_keys.insert( + ks.insert_at( (PrivateKeyTarget::PrivateKeyOnMainIdentity, owner.id()), ( QualifiedIdentityPublicKey::from(owner), diff --git a/src/ui/masternodes/mod.rs b/src/ui/masternodes/mod.rs index 01edf549b..07a6d13fd 100644 --- a/src/ui/masternodes/mod.rs +++ b/src/ui/masternodes/mod.rs @@ -6,11 +6,13 @@ //! are page-scoped and never leak into the everyday-user surfaces (FR-6, B1). //! //! The gate covers the screens, not this module's shared key helpers -//! ([`role_label_and_tip`], [`manage_keys_labels`], [`identity_keys`], -//! [`key_filed_at`]): those name, enumerate and resolve the keys of any identity -//! and are used from ungated surfaces — the identity keys list and the -//! recovery-offer component — so that one key cannot be called two different -//! things, or reported as saved on one screen and missing on another. +//! ([`role_label_and_tip`], [`manage_keys_labels`], [`identity_keys`]): those +//! name and enumerate the keys of any identity and are used from ungated +//! surfaces — the identity keys list and the recovery-offer component — so that +//! one key cannot be called two different things on two screens. Whether a key +//! is *held* is resolved by +//! [`KeyStorage::candidates`](crate::model::qualified_identity::encrypted_key_storage::KeyStorage::candidates), +//! shared for the same reason. pub mod card; pub mod detail_screen; @@ -242,9 +244,10 @@ pub(crate) fn key_role_label( /// keys identically. The target paired here is the *structural* one — which /// identity's key map the key came from — which is only half of pairing a public /// key with the private material the device may hold for it. Resolving that is -/// [`key_filed_at`], shared for the same reason: enumerating alike while -/// resolving differently is how the two surfaces came to disagree about whether -/// one key was saved on this device. +/// [`KeyStorage::candidates`](crate::model::qualified_identity::encrypted_key_storage::KeyStorage::candidates), +/// which every surface shares: enumerating alike while resolving differently is +/// how the two surfaces came to disagree about whether one key was saved on this +/// device. pub fn identity_keys( identity: &QualifiedIdentity, ) -> Vec<(PrivateKeyTarget, dash_sdk::platform::IdentityPublicKey)> { @@ -265,98 +268,6 @@ pub fn identity_keys( keys } -/// Whether a stored public key and a live one are the same key. -/// -/// Compares every field except `disabled_at`. A Platform identity public key is -/// immutable once added, with that single exception: disabling one rewrites that -/// field. The stored copy is a snapshot taken when the private half was saved, so -/// plain `==` stops matching as soon as a key is disabled or rotated, and a key -/// this device demonstrably holds is reported as missing. -/// -/// Comparing only the id and the key material would fix that and reopen a worse -/// hole in the other direction. Both are shared by construction where it matters: -/// `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 hands over private material the -/// clicked key does not own — so this excludes the one field that legitimately -/// moves, and nothing else. -fn same_key( - stored: &dash_sdk::platform::IdentityPublicKey, - live: &dash_sdk::platform::IdentityPublicKey, -) -> bool { - use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; - use dash_sdk::platform::IdentityPublicKey; - - let IdentityPublicKey::V0(stored) = stored; - let IdentityPublicKey::V0(live) = live; - // Destructured exhaustively, and without `..`, on purpose: a field added - // upstream must break this build rather than be silently ignored. A new - // field that distinguishes two keys would otherwise leave this reporting a - // match where there is none — which is how a key's private material ends up - // attributed to a different key. Whoever adds it decides here whether it - // identifies a key or, like `disabled_at`, merely describes its state. - let IdentityPublicKeyV0 { - id, - purpose, - security_level, - contract_bounds, - key_type, - read_only, - data, - // The one field Platform lets move after a key is added: disabling a key - // rewrites it, and the stored snapshot was taken before that happened. - disabled_at: _, - } = stored; - - *id == live.id - && *purpose == live.purpose - && *security_level == live.security_level - && *contract_bounds == live.contract_bounds - && *key_type == live.key_type - && *read_only == live.read_only - && *data == live.data -} - -/// Which store `key`'s private half is actually in, or `None` if this device -/// holds no material for it. -/// -/// `structural` is the target [`identity_keys`] paired the key with — which -/// identity's key map it was found in. Two conventions for that target are in use -/// in this codebase: the structural one, and `impl From for -/// PrivateKeyTarget`, which files by purpose alone. Real installs hold material -/// written under each, so both are tried. -/// -/// Shared by every "Manage keys" surface on purpose. [`identity_keys`] enumerates -/// the same keys for all of them and this resolves held-ness the same way for all -/// of them; splitting either one is how the identity keys list and the masternode -/// detail view came to disagree about whether one key was saved on this device. -/// -/// This is a read. Looking in the second place can only find material that is -/// already there, so it corrects a false "not saved on this device" without -/// moving anything; reconciling the two conventions on the write path is a -/// migration, tracked separately. -/// -/// Each candidate has to hold *this* public key, not merely have its slot filled: -/// a voter identity's own key can share a key id with a main-identity key, so an -/// occupied slot proves nothing about whose material is in it. -/// -/// Reads the stored public half only, never the private one: fetching the entry -/// would clone the raw private key out of the vault unscrubbed, and this runs -/// every frame for every key. -pub fn key_filed_at( - identity: &QualifiedIdentity, - structural: &PrivateKeyTarget, - key: &dash_sdk::platform::IdentityPublicKey, -) -> Option { - let derived: PrivateKeyTarget = key.purpose().into(); - [structural.clone(), derived].into_iter().find(|candidate| { - identity - .private_keys - .public_key_for(&(candidate.clone(), key.id())) - .is_some_and(|stored| same_key(&stored.identity_public_key, key)) - }) -} - /// Button labels (and DIP-3-aligned tooltips) for a "Manage keys" list, one /// per entry of `keys`, in order. /// @@ -731,127 +642,4 @@ mod tests { ] ); } - - /// Build a `QualifiedIdentity` holding the private half of `stored` filed at - /// `at`, whose on-chain key set is `live`. - fn identity_holding( - at: PrivateKeyTarget, - stored: dash_sdk::platform::IdentityPublicKey, - live: &[dash_sdk::platform::IdentityPublicKey], - ) -> QualifiedIdentity { - use crate::model::qualified_identity::encrypted_key_storage::{KeyStorage, PrivateKeyData}; - use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; - use crate::model::qualified_identity::{IdentityStatus, IdentityType}; - use dash_sdk::dpp::identity::Identity; - use dash_sdk::dpp::version::PlatformVersion; - use dash_sdk::platform::Identifier; - use std::collections::BTreeMap; - - let identity = Identity::new_with_id_and_keys( - Identifier::from([0x60u8; 32]), - live.iter().map(|k| (k.id(), k.clone())).collect(), - PlatformVersion::latest(), - ) - .expect("identity with keys"); - QualifiedIdentity { - identity, - associated_voter_identity: None, - associated_operator_identity: None, - associated_owner_key_id: None, - identity_type: IdentityType::Masternode, - alias: Some("filed-at".to_string()), - private_keys: KeyStorage { - private_keys: BTreeMap::from([( - (at, stored.id()), - ( - QualifiedIdentityPublicKey::from(stored), - PrivateKeyData::Clear([0x60; 32]), - ), - )]), - }, - dpns_names: vec![], - associated_wallets: BTreeMap::new(), - secret_access: None, - wallet_index: None, - top_ups: BTreeMap::new(), - status: IdentityStatus::Active, - network: dash_sdk::dpp::dashcore::Network::Testnet, - } - } - - /// The resolution rule every "Manage keys" surface shares, in one place. - /// - /// Two target conventions are in use, so both are tried; a key must be - /// matched by its own public half rather than by an occupied slot; and - /// `disabled_at` must not break the match, because it is the one field - /// Platform lets move after a key is added. - #[test] - fn held_material_is_found_under_either_convention_and_only_for_the_right_key() { - use dash_sdk::dpp::identity::Purpose; - - // Filed structurally, where `identity_keys` says it is. - let key = mn_key(0, Purpose::AUTHENTICATION, false); - let identity = identity_holding( - PrivateKeyTarget::PrivateKeyOnMainIdentity, - key.clone(), - std::slice::from_ref(&key), - ); - assert_eq!( - key_filed_at(&identity, &PrivateKeyTarget::PrivateKeyOnMainIdentity, &key), - Some(PrivateKeyTarget::PrivateKeyOnMainIdentity), - "material filed where the key structurally sits must be found" - ); - - // Filed under the purpose-derived convention instead: a voting key on - // the main identity, entered by hand. Reported as unheld before the - // fallback existed. - let voting = mn_key(0, Purpose::VOTING, false); - let identity = identity_holding( - PrivateKeyTarget::PrivateKeyOnVoterIdentity, - voting.clone(), - std::slice::from_ref(&voting), - ); - assert_eq!( - key_filed_at( - &identity, - &PrivateKeyTarget::PrivateKeyOnMainIdentity, - &voting - ), - Some(PrivateKeyTarget::PrivateKeyOnVoterIdentity), - "material filed by purpose derivation must still be found" - ); - - // A *different* key at the same id must not match. These two share - // `id` and `data`, so purpose is the only thing telling them apart — - // matching here would report one key as held on the strength of - // another's private half. - assert_eq!( - key_filed_at( - &identity, - &PrivateKeyTarget::PrivateKeyOnMainIdentity, - &mn_key(0, Purpose::AUTHENTICATION, false) - ), - None, - "an occupied slot proves nothing about whose material is in it" - ); - - // Disabling a key on chain does not remove its private half from this - // device, so the stored snapshot must still match the live key. - let disabled = mn_key(0, Purpose::AUTHENTICATION, true); - let identity = identity_holding( - PrivateKeyTarget::PrivateKeyOnMainIdentity, - mn_key(0, Purpose::AUTHENTICATION, false), - std::slice::from_ref(&disabled), - ); - assert_eq!( - key_filed_at( - &identity, - &PrivateKeyTarget::PrivateKeyOnMainIdentity, - &disabled - ), - Some(PrivateKeyTarget::PrivateKeyOnMainIdentity), - "`disabled_at` is the one field that legitimately moves, so it must \ - not break the match" - ); - } } diff --git a/src/ui/tokens/tokens_screen/mod.rs b/src/ui/tokens/tokens_screen/mod.rs index 161925fcc..ea4b48f6f 100644 --- a/src/ui/tokens/tokens_screen/mod.rs +++ b/src/ui/tokens/tokens_screen/mod.rs @@ -3293,9 +3293,7 @@ mod tests { associated_owner_key_id: None, identity_type: crate::model::qualified_identity::IdentityType::User, alias: None, - private_keys: KeyStorage { - private_keys: BTreeMap::new(), - }, + private_keys: KeyStorage::default(), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, @@ -3595,9 +3593,7 @@ mod tests { associated_owner_key_id: None, identity_type: crate::model::qualified_identity::IdentityType::User, alias: None, - private_keys: KeyStorage { - private_keys: BTreeMap::new(), - }, + private_keys: KeyStorage::default(), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, @@ -3711,9 +3707,7 @@ mod tests { associated_owner_key_id: None, identity_type: crate::model::qualified_identity::IdentityType::User, alias: None, - private_keys: KeyStorage { - private_keys: BTreeMap::new(), - }, + private_keys: KeyStorage::default(), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, diff --git a/src/ui/tokens/tokens_screen/token_creator.rs b/src/ui/tokens/tokens_screen/token_creator.rs index 5ca7bd11f..ef566e438 100644 --- a/src/ui/tokens/tokens_screen/token_creator.rs +++ b/src/ui/tokens/tokens_screen/token_creator.rs @@ -74,7 +74,7 @@ impl TokensScreen { // Identity and key selection ui.add_space(10.0); let all_identities = match self.app_context.load_local_user_identities() { - Ok(identities) => identities.into_iter().filter(|qi| !qi.private_keys.private_keys.is_empty()).collect::>(), + Ok(identities) => identities.into_iter().filter(|qi| !qi.private_keys.is_empty()).collect::>(), Err(error) => { tracing::error!(?error, "Token-creator identity loading failed"); ui.colored_label( diff --git a/tests/backend-e2e/framework/fixtures.rs b/tests/backend-e2e/framework/fixtures.rs index 4444a7a80..1f6c89fbc 100644 --- a/tests/backend-e2e/framework/fixtures.rs +++ b/tests/backend-e2e/framework/fixtures.rs @@ -314,7 +314,7 @@ pub fn find_authentication_public_key( SecurityLevel::HIGH, SecurityLevel::MASTER, ] { - for ((target, _key_id), (qualified_key, _)) in qi.private_keys.private_keys.iter() { + for ((target, _key_id), (qualified_key, _)) in qi.private_keys.iter() { if *target != PrivateKeyTarget::PrivateKeyOnMainIdentity { continue; } diff --git a/tests/backend-e2e/identity_in_vault_sign.rs b/tests/backend-e2e/identity_in_vault_sign.rs index 18c9707de..9914dc955 100644 --- a/tests/backend-e2e/identity_in_vault_sign.rs +++ b/tests/backend-e2e/identity_in_vault_sign.rs @@ -84,14 +84,12 @@ async fn ts_sign_e2e_01_in_vault_identity_signs_and_broadcasts() { // migrated keys — no resident plaintext. assert!( qi.private_keys - .private_keys .values() .all(|(_, d)| !matches!(d, PrivateKeyData::Clear(_) | PrivateKeyData::AlwaysClear(_))), "no plaintext identity key may remain resident after migration" ); assert!( qi.private_keys - .private_keys .values() .any(|(_, d)| matches!(d, PrivateKeyData::InVault)), "migrated keys must be InVault placeholders" @@ -233,7 +231,6 @@ async fn materialize_master_key_as_clear( let network = ctx.app_context.network(); let (map_key, master_pub) = qi - .private_keys .private_keys .iter() .find_map(|(map_key, (pub_key, _))| { diff --git a/tests/kittest/identity_home.rs b/tests/kittest/identity_home.rs index 6c83124bd..7c2e2301c 100644 --- a/tests/kittest/identity_home.rs +++ b/tests/kittest/identity_home.rs @@ -62,7 +62,7 @@ fn seed_identity( associated_owner_key_id: None, identity_type: IdentityType::User, alias: Some("Home test identity".to_string()), - private_keys: KeyStorage { private_keys }, + private_keys: KeyStorage::from(private_keys), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, diff --git a/tests/kittest/key_info_screen.rs b/tests/kittest/key_info_screen.rs index 75844dd14..24232b00d 100644 --- a/tests/kittest/key_info_screen.rs +++ b/tests/kittest/key_info_screen.rs @@ -142,7 +142,7 @@ fn a_write_that_lands_while_key_info_is_open_survives_the_next_key_edit() { .expect("read the record") .expect("record stored"); record.identity.add_public_key(restored.clone()); - record.private_keys.private_keys.insert( + record.private_keys.insert_at( (MAIN, restored.id()), ( QualifiedIdentityPublicKey::from(restored.clone()), @@ -174,8 +174,7 @@ fn a_write_that_lands_while_key_info_is_open_survives_the_next_key_edit() { .expect("read back") .expect("still stored") .private_keys - .private_keys - .contains_key(&(MAIN, restored.id())), + .has(&(MAIN, restored.id())), "a key edit on this screen must not erase a key written while it was open", ); }); diff --git a/tests/kittest/keys_screen.rs b/tests/kittest/keys_screen.rs index 5b33668f9..53c39ceb1 100644 --- a/tests/kittest/keys_screen.rs +++ b/tests/kittest/keys_screen.rs @@ -133,15 +133,13 @@ fn identity_holding_key( associated_owner_key_id: None, identity_type: IdentityType::User, alias: Some("held-key".to_string()), - private_keys: KeyStorage { - private_keys: BTreeMap::from([( - (target, public_key.id()), - ( - QualifiedIdentityPublicKey::from(public_key), - PrivateKeyData::Clear([id_byte; 32]), - ), - )]), - }, + private_keys: KeyStorage::from(BTreeMap::from([( + (target, public_key.id()), + ( + QualifiedIdentityPublicKey::from(public_key), + PrivateKeyData::Clear([id_byte; 32]), + ), + )])), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, @@ -947,15 +945,13 @@ fn a_same_numbered_key_on_the_voter_identity_is_not_mistaken_for_this_one() { associated_owner_key_id: None, identity_type: IdentityType::Masternode, alias: Some("id-collision".to_string()), - private_keys: KeyStorage { - private_keys: BTreeMap::from([( - (PrivateKeyTarget::PrivateKeyOnVoterIdentity, voter_key.id()), - ( - QualifiedIdentityPublicKey::from(voter_key), - PrivateKeyData::Clear([0x42; 32]), - ), - )]), - }, + private_keys: KeyStorage::from(BTreeMap::from([( + (PrivateKeyTarget::PrivateKeyOnVoterIdentity, voter_key.id()), + ( + QualifiedIdentityPublicKey::from(voter_key), + PrivateKeyData::Clear([0x42; 32]), + ), + )])), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, @@ -1029,15 +1025,13 @@ fn a_disabled_key_whose_private_half_is_saved_is_still_reported_as_held() { associated_owner_key_id: None, identity_type: IdentityType::User, alias: Some("disabled-but-held".to_string()), - private_keys: KeyStorage { - private_keys: BTreeMap::from([( - (PrivateKeyTarget::PrivateKeyOnMainIdentity, snapshot.id()), - ( - QualifiedIdentityPublicKey::from(snapshot), - PrivateKeyData::Clear([0x43; 32]), - ), - )]), - }, + private_keys: KeyStorage::from(BTreeMap::from([( + (PrivateKeyTarget::PrivateKeyOnMainIdentity, snapshot.id()), + ( + QualifiedIdentityPublicKey::from(snapshot), + PrivateKeyData::Clear([0x43; 32]), + ), + )])), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, diff --git a/tests/kittest/masternode_tab.rs b/tests/kittest/masternode_tab.rs index ee335ad9f..ec2688da0 100644 --- a/tests/kittest/masternode_tab.rs +++ b/tests/kittest/masternode_tab.rs @@ -151,15 +151,13 @@ fn seed_node_with_unprotected_held_key(app_context: &Arc, byte: u8, pv, ) .expect("masternode identity with key"); - let private_keys = KeyStorage { - private_keys: BTreeMap::from([( - (PrivateKeyTarget::PrivateKeyOnMainIdentity, key.id()), - ( - QualifiedIdentityPublicKey::from(key), - PrivateKeyData::Clear([byte; 32]), - ), - )]), - }; + let private_keys = KeyStorage::from(BTreeMap::from([( + (PrivateKeyTarget::PrivateKeyOnMainIdentity, key.id()), + ( + QualifiedIdentityPublicKey::from(key), + PrivateKeyData::Clear([byte; 32]), + ), + )])); let node_qi = QualifiedIdentity { identity, associated_voter_identity: None, @@ -1126,15 +1124,13 @@ fn seed_node_with_non_voting_purpose_voter_key( associated_owner_key_id: None, identity_type: IdentityType::Masternode, alias: Some(alias.to_string()), - private_keys: KeyStorage { - private_keys: BTreeMap::from([( - (PrivateKeyTarget::PrivateKeyOnVoterIdentity, voter_key.id()), - ( - QualifiedIdentityPublicKey::from(voter_key), - PrivateKeyData::Clear([byte; 32]), - ), - )]), - }, + private_keys: KeyStorage::from(BTreeMap::from([( + (PrivateKeyTarget::PrivateKeyOnVoterIdentity, voter_key.id()), + ( + QualifiedIdentityPublicKey::from(voter_key), + PrivateKeyData::Clear([byte; 32]), + ), + )])), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, @@ -1265,16 +1261,14 @@ fn seed_node_with_purpose_filed_voting_key(app_context: &Arc, byte: associated_owner_key_id: None, identity_type: IdentityType::Masternode, alias: Some(alias.to_string()), - private_keys: KeyStorage { - private_keys: BTreeMap::from([( - // Filed by purpose derivation, not structurally. - (PrivateKeyTarget::PrivateKeyOnVoterIdentity, voting_key.id()), - ( - QualifiedIdentityPublicKey::from(voting_key), - PrivateKeyData::Clear([byte; 32]), - ), - )]), - }, + private_keys: KeyStorage::from(BTreeMap::from([( + // Filed by purpose derivation, not structurally. + (PrivateKeyTarget::PrivateKeyOnVoterIdentity, voting_key.id()), + ( + QualifiedIdentityPublicKey::from(voting_key), + PrivateKeyData::Clear([byte; 32]), + ), + )])), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None, diff --git a/tests/kittest/withdraw_screen.rs b/tests/kittest/withdraw_screen.rs index ea40c7e4a..57a9dda91 100644 --- a/tests/kittest/withdraw_screen.rs +++ b/tests/kittest/withdraw_screen.rs @@ -104,7 +104,7 @@ fn build_identity( associated_owner_key_id: None, identity_type, alias: None, - private_keys: KeyStorage { private_keys }, + private_keys: KeyStorage::from(private_keys), dpns_names: vec![], associated_wallets: BTreeMap::new(), secret_access: None,