From b32c83743ee5a9db11bedf82dcfcb0ff664e3cc8 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:07:42 +0000 Subject: [PATCH 01/15] fix(sdk): refuse identity updates that disable the last valid authentication key of a level DIP-11: "Keys can only be disabled if another valid key is enabled in the same security level. This can happen atomically in the same state transition." Consensus enforces this for MASTER only, so every client write path could disable the last CRITICAL/HIGH/MEDIUM authentication key. dash-sdk gains `UpdateIdentity::update_identity_keys`, which checks the rule before signing, claims the next revision, fetches the nonce, builds and broadcasts. `check_keys_can_be_disabled` exposes the same rule as a query. A level is a minimum: a stronger non-MASTER key stands in for a weaker one; MASTER signs identity updates only, so it stands in for nothing but MASTER. Valid means enabled and unexpired; keys added in the same update count. The refusal is the typed `dash_sdk::Error::LastAuthenticationKey`. The platform-wallet update methods and wasm-sdk `identityUpdate` route through the SDK method, so no caller has to pre-check anything. Co-Authored-By: Claude Opus 5.5 --- .../src/wallet/identity/network/update.rs | 226 ++++++---- packages/rs-sdk/src/error.rs | 6 + packages/rs-sdk/src/platform/transition.rs | 1 + .../platform/transition/update_identity.rs | 399 ++++++++++++++++++ packages/wasm-sdk/src/error.rs | 7 + .../src/state_transitions/identity.rs | 57 +-- 6 files changed, 574 insertions(+), 122 deletions(-) create mode 100644 packages/rs-sdk/src/platform/transition/update_identity.rs diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs index 9806b257692..941b9ad3e57 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs @@ -18,6 +18,8 @@ use dpp::ProtocolError; use dpp::identity::signer::Signer; use dash_sdk::platform::transition::put_settings::PutSettings; +use dash_sdk::platform::transition::update_identity::UpdateIdentity; +use dpp::state_transition::proof_result::StateTransitionProofResult; use crate::error::PlatformWalletError; @@ -71,6 +73,11 @@ impl IdentityWallet { /// identity-update state transition requires (DPP gates this on /// MASTER specifically — HIGH/CRITICAL aren't accepted). /// + /// An update that would disable the last valid authentication key of a + /// security level is refused before signing with + /// `PlatformWalletError::Sdk(dash_sdk::Error::LastAuthenticationKey(_))` + /// (DIP-11); keys added in the same update count as valid. + /// /// NOTE: callers that ADD keys via `add_public_keys` are /// responsible for pre-persisting the new keys' private material /// to whatever store the supplied signer reads from (iOS Keychain @@ -98,16 +105,11 @@ impl IdentityWallet { where S: Signer + Send + Sync, { - use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; - use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; - use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; - use dpp::state_transition::proof_result::StateTransitionProofResult; - // Snapshot the local identity + its `identity_index` (needed // for the derivation breadcrumb on the post-broadcast apply // pass below). Read lock only — the broadcast itself doesn't // touch local state. - let (mut identity, identity_index) = { + let (identity, identity_index) = { let wm = self.wallet_manager.read().await; let info = wm.get_wallet_info(&self.wallet_id).ok_or_else(|| { crate::error::PlatformWalletError::WalletNotFound( @@ -130,10 +132,6 @@ impl IdentityWallet { (identity, index) }; - // Increment revision for the update transition. - let original_revision = identity.revision(); - identity.set_revision(original_revision + 1); - // Pick the MASTER signing key — DPP requires identity update // transitions to be authorized by MASTER specifically. let master_key_id = identity @@ -151,49 +149,27 @@ impl IdentityWallet { ) })?; - let identity_nonce = self - .sdk - .get_identity_nonce(identity.id(), true, settings) - .await?; - - let user_fee_increase = settings - .and_then(|s| s.user_fee_increase) - .unwrap_or_default(); - - // `try_from_identity_with_signer` consumes the keys vec, so - // clone before handing it off — we need the originals to - // apply locally after the broadcast succeeds. + // The update consumes the keys vecs, so clone before handing + // them off — we need the originals to apply locally after the + // broadcast succeeds. let added_keys_for_local_apply = add_public_keys.clone(); let disabled_ids_for_local_apply = disable_public_keys.clone(); - let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( - &identity, - &master_key_id, - add_public_keys, - disable_public_keys, - identity_nonce, - user_fee_increase, - &SignerRef(signer), - self.sdk.version(), - None, - ) - .await - .map_err(|e| { - PlatformWalletError::InvalidIdentityData(format!( - "Failed to create identity update transition: {}", - e - )) - })?; - - state_transition - .broadcast_and_wait::(&self.sdk, settings) - .await - .map_err(|e| { - PlatformWalletError::InvalidIdentityData(format!( - "Failed to broadcast identity update: {}", - e - )) - })?; + // The SDK claims the next revision and refuses, before signing, + // an update that disables the last valid authentication key of + // a security level (DIP-11). Its errors stay typed as + // `PlatformWalletError::Sdk`, so the FFI keeps the consensus code + // and the signer-unavailable classification. + identity + .update_identity_keys( + &self.sdk, + master_key_id, + add_public_keys, + disable_public_keys, + &SignerRef(signer), + settings, + ) + .await?; // Post-broadcast local apply. // @@ -283,6 +259,11 @@ impl IdentityWallet { /// caller receives the chain's error. See /// `docs/protocol/authentication-key-limits.md`. /// + /// `identity` is the identity as Platform holds it: the update claims + /// the next revision. An update that would disable the last valid + /// authentication key of a security level is refused before signing + /// with [`dash_sdk::Error::LastAuthenticationKey`] (DIP-11). + /// /// Returns the [`StateTransitionProofResult`] from the broadcast so callers /// can inspect proof-verified outcomes (e.g. updated keys, balance). pub async fn update_identity_with_signer>( @@ -293,42 +274,123 @@ impl IdentityWallet { disable_public_keys: Vec, signer: &S, settings: Option, - ) -> Result - { - use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; - use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; - use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; + ) -> Result { + identity + .update_identity_keys( + &self.sdk, + *master_key_id, + add_public_keys, + disable_public_keys, + signer, + settings, + ) + .await + } +} - // Get identity nonce from Platform. - let identity_nonce = self - .sdk - .get_identity_nonce(identity.id(), true, settings) - .await?; +#[cfg(test)] +mod tests { + use super::*; + use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dpp::identity::v0::IdentityV0; + use std::collections::BTreeMap; - let user_fee_increase = settings - .and_then(|s| s.user_fee_increase) - .unwrap_or_default(); + /// Signs nothing; holds every key so the MASTER key is selectable. + #[derive(Debug)] + struct HoldsEveryKey; + + #[async_trait] + impl Signer for HoldsEveryKey { + async fn sign( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(ProtocolError::Generic( + "not signing in this test".to_string(), + )) + } - // Build the update transition. - let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( - identity, - master_key_id, - add_public_keys, - disable_public_keys, - identity_nonce, - user_fee_increase, - signer, - self.sdk.version(), - None, - ) - .await - .map_err(dash_sdk::Error::Protocol)?; + async fn sign_create_witness( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(ProtocolError::Generic( + "not signing in this test".to_string(), + )) + } - // Broadcast and wait for confirmation. - let result = state_transition - .broadcast_and_wait(&self.sdk, settings) - .await?; + fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { + true + } + } + + /// MASTER 0, CRITICAL 1, HIGH 2: one authentication key per level. + fn identity() -> Identity { + let keys = [ + (0, SecurityLevel::MASTER), + (1, SecurityLevel::CRITICAL), + (2, SecurityLevel::HIGH), + ] + .map(|(id, security_level)| { + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose: Purpose::AUTHENTICATION, + security_level, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new(vec![id as u8 + 2; 33]), + disabled_at: None, + }) + }); + IdentityV0 { + id: Identifier::from([1; 32]), + public_keys: keys + .into_iter() + .map(|key| (key.id(), key)) + .collect::>(), + balance: 0, + revision: 0, + } + .into() + } + + /// The mock SDK answers no request, so a refusal that reached the network + /// would surface as a different error. + #[tokio::test] + async fn should_refuse_disabling_the_last_critical_key_before_sending_anything() { + let (manager, wallet_id) = crate::test_support::test_platform_wallet_manager().await; + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + { + let persister = + WalletPersister::new(wallet_id, std::sync::Arc::new(NoPlatformPersistence)); + let mut wm = manager.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + info.identity_manager + .add_identity(identity(), 0, wallet_id, &persister) + .expect("add identity"); + } - Ok(result) + let result = wallet + .identity() + .update_identity_with_external_signer( + &Identifier::from([1; 32]), + vec![], + vec![1], + &HoldsEveryKey, + None, + ) + .await; + assert!( + matches!( + result, + Err(PlatformWalletError::Sdk(dash_sdk::Error::LastAuthenticationKey(ref refusal))) + if refusal.key_id == 1 && refusal.security_level == SecurityLevel::CRITICAL + ), + "{result:?}" + ); } } diff --git a/packages/rs-sdk/src/error.rs b/packages/rs-sdk/src/error.rs index d070903ac46..8e2ece2ba68 100644 --- a/packages/rs-sdk/src/error.rs +++ b/packages/rs-sdk/src/error.rs @@ -1,5 +1,6 @@ //! Definitions of errors use crate::platform::encrypted_for::EncryptedForError; +use crate::platform::transition::update_identity::LastAuthenticationKeyError; use dapi_grpc::platform::v0::StateTransitionBroadcastError as StateTransitionBroadcastErrorProto; use dapi_grpc::tonic::Code; pub use dash_context_provider::ContextProviderError; @@ -137,6 +138,11 @@ pub enum Error { #[error(transparent)] EncryptedFor(#[from] EncryptedForError), + /// An identity update would disable the last valid authentication key of a security + /// level, which DIP-11 forbids. Refused before anything was signed. + #[error(transparent)] + LastAuthenticationKey(#[from] LastAuthenticationKeyError), + /// The request uses a feature that not every node of the network serves yet, so the /// SDK refused it without sending it: nodes of an older release would answer /// UNIMPLEMENTED or a different question, and get banned for it. diff --git a/packages/rs-sdk/src/platform/transition.rs b/packages/rs-sdk/src/platform/transition.rs index 3a99af47952..13c0dd2b127 100644 --- a/packages/rs-sdk/src/platform/transition.rs +++ b/packages/rs-sdk/src/platform/transition.rs @@ -41,6 +41,7 @@ pub mod transfer_to_addresses; mod txid; #[cfg(feature = "shielded")] pub mod unshield; +pub mod update_identity; pub mod update_identity_key_limits; pub mod update_price_of_document; pub(crate) mod validation; diff --git a/packages/rs-sdk/src/platform/transition/update_identity.rs b/packages/rs-sdk/src/platform/transition/update_identity.rs new file mode 100644 index 00000000000..f32436b5dfb --- /dev/null +++ b/packages/rs-sdk/src/platform/transition/update_identity.rs @@ -0,0 +1,399 @@ +//! Add and disable an identity's public keys. +//! +//! [`UpdateIdentity::update_identity_keys`] refuses, before anything is signed, an update that +//! disables the last valid authentication key of a security level. DIP-11: "Keys can only be +//! disabled if another valid key is enabled in the same security level. This can happen +//! atomically in the same state transition." Consensus enforces this for MASTER keys only, so +//! for every other level the client is the only enforcer. [`check_keys_can_be_disabled`] is the +//! same rule as a query, for an interface that wants to offer only the keys it may disable. +//! +//! ```ignore +//! let result = identity +//! .update_identity_keys(&sdk, master_key_id, vec![new_key], vec![old_key_id], &signer, None) +//! .await?; +//! ``` + +use dpp::identity::accessors::{IdentityGettersV0, IdentitySettersV0}; +use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; +use dpp::identity::identity_public_key::accessors::v1::IdentityPublicKeyGettersV1; +use dpp::identity::signer::Signer; +use dpp::identity::{Identity, IdentityPublicKey, KeyID, Purpose, SecurityLevel, TimestampMillis}; +use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; +use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; +use dpp::state_transition::proof_result::StateTransitionProofResult; + +use crate::platform::transition::broadcast::BroadcastStateTransition; +use crate::platform::transition::put_settings::PutSettings; +use crate::platform::transition::validation::ensure_valid_state_transition_structure; +use crate::{Error, Sdk}; + +/// An identity update would disable the last valid authentication key of a security level. +/// +/// A security level is a minimum: a key of a stronger level stands in for it, except MASTER, +/// which signs identity updates only. So a CRITICAL key keeps a HIGH key disableable, a MASTER +/// key does not, and a MASTER key needs another MASTER key. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +#[error( + "cannot disable authentication key {key_id}: no other enabled, unexpired authentication key \ + at security level {security_level} or one standing in for it would remain" +)] +pub struct LastAuthenticationKeyError { + /// The key the update would disable. + pub key_id: KeyID, + /// The security level that would be left without a valid authentication key. + pub security_level: SecurityLevel, +} + +/// Whether an identity update that adds `add_public_keys` and disables `disable_public_keys` +/// keeps a valid authentication key at the security level of every authentication key it +/// disables, at the time `now_ms` (Unix milliseconds). +/// +/// A valid key is enabled and not expired at `now_ms`. The keys the identity keeps count, and +/// so do the keys the same update adds. Ids the identity does not hold are not judged here: +/// Platform refuses them. +pub fn check_keys_can_be_disabled( + identity: &Identity, + add_public_keys: &[IdentityPublicKey], + disable_public_keys: &[KeyID], + now_ms: TimestampMillis, +) -> Result<(), LastAuthenticationKeyError> { + let remaining_valid_keys = || { + identity + .public_keys() + .values() + .filter(|key| !disable_public_keys.contains(&key.id())) + .chain(add_public_keys) + .filter(|key| { + key.purpose() == Purpose::AUTHENTICATION + && key.disabled_at().is_none() + && !key.is_expired_at(now_ms) + }) + }; + for key_id in disable_public_keys { + let Some(key) = identity.public_keys().get(key_id) else { + continue; + }; + if key.purpose() != Purpose::AUTHENTICATION { + continue; + } + let security_level = key.security_level(); + if !remaining_valid_keys() + .any(|remaining| stands_in_for(remaining.security_level(), security_level)) + { + return Err(LastAuthenticationKeyError { + key_id: *key_id, + security_level, + }); + } + } + Ok(()) +} + +/// Whether a key at `candidate` satisfies a requirement for `required`: the same level, or a +/// stronger one other than MASTER, which signs nothing but identity updates. +fn stands_in_for(candidate: SecurityLevel, required: SecurityLevel) -> bool { + candidate == required + || (candidate != SecurityLevel::MASTER && candidate.stronger_security_than(required)) +} + +/// The current wall-clock time in Unix milliseconds, on native targets and in the browser. +fn now_ms() -> Result { + #[cfg(not(target_arch = "wasm32"))] + { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|elapsed| elapsed.as_millis() as TimestampMillis) + .map_err(|e| Error::Generic(format!("system time is before the Unix epoch: {e}"))) + } + #[cfg(target_arch = "wasm32")] + { + Ok(js_sys::Date::now() as TimestampMillis) + } +} + +#[async_trait::async_trait] +pub trait UpdateIdentity { + /// Adds `add_public_keys` to this identity and disables the keys `disable_public_keys`, + /// signing with its MASTER key `master_key_id`, and waits for the proved result. + /// + /// `self` is the identity as Platform holds it; the update claims the next revision. + /// Refused before anything is signed, with [`Error::LastAuthenticationKey`], when it would + /// disable the last valid authentication key of a security level: see + /// [`check_keys_can_be_disabled`]. + /// + /// Keys in `add_public_keys` carry their final ids. Their private keys must be reachable + /// through `signer`, which signs each unique-type key's proof of possession as well as the + /// transition. + async fn update_identity_keys>( + &self, + sdk: &Sdk, + master_key_id: KeyID, + add_public_keys: Vec, + disable_public_keys: Vec, + signer: &S, + settings: Option, + ) -> Result; +} + +#[async_trait::async_trait] +impl UpdateIdentity for Identity { + async fn update_identity_keys>( + &self, + sdk: &Sdk, + master_key_id: KeyID, + add_public_keys: Vec, + disable_public_keys: Vec, + signer: &S, + settings: Option, + ) -> Result { + check_keys_can_be_disabled(self, &add_public_keys, &disable_public_keys, now_ms()?)?; + + let mut identity = self.clone(); + let revision = identity + .revision() + .checked_add(1) + .ok_or_else(|| Error::Generic("identity revision overflows".to_string()))?; + identity.set_revision(revision); + + let identity_nonce = sdk + .get_identity_nonce(identity.id(), true, settings) + .await?; + let user_fee_increase = settings + .and_then(|settings| settings.user_fee_increase) + .unwrap_or_default(); + let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( + &identity, + &master_key_id, + add_public_keys, + disable_public_keys, + identity_nonce, + user_fee_increase, + signer, + sdk.version(), + None, + ) + .await?; + ensure_valid_state_transition_structure(&state_transition, sdk.version())?; + + state_transition.broadcast_and_wait(sdk, settings).await + } +} + +#[cfg(test)] +mod tests { + use super::*; + use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; + use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dpp::identity::v0::IdentityV0; + use dpp::identity::KeyType; + use dpp::platform_value::{BinaryData, Identifier}; + use std::collections::BTreeMap; + + const NOW_MS: TimestampMillis = 1_800_000_000_000; + + fn key(id: KeyID, purpose: Purpose, security_level: SecurityLevel) -> IdentityPublicKey { + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose, + security_level, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new(vec![id as u8; 33]), + disabled_at: None, + }) + } + + fn authentication_key(id: KeyID, security_level: SecurityLevel) -> IdentityPublicKey { + key(id, Purpose::AUTHENTICATION, security_level) + } + + fn identity(keys: Vec) -> Identity { + IdentityV0 { + id: Identifier::from([1; 32]), + public_keys: keys + .into_iter() + .map(|key| (key.id(), key)) + .collect::>(), + balance: 0, + revision: 0, + } + .into() + } + + /// MASTER 0, CRITICAL 1, HIGH 2, MEDIUM 3: one authentication key per level. + fn one_key_per_level() -> Identity { + identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + authentication_key(2, SecurityLevel::HIGH), + authentication_key(3, SecurityLevel::MEDIUM), + ]) + } + + #[test] + fn should_refuse_disabling_the_only_critical_key() { + let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[1], NOW_MS); + assert_eq!( + result, + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + } + + #[test] + fn should_refuse_disabling_the_only_master_key_without_adding_one() { + let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[0], NOW_MS); + assert_eq!( + result, + Err(LastAuthenticationKeyError { + key_id: 0, + security_level: SecurityLevel::MASTER, + }) + ); + } + + #[test] + fn should_let_a_stronger_non_master_key_stand_in_for_a_weaker_level() { + // CRITICAL 1 still meets HIGH and MEDIUM requirements. + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[], &[2, 3], NOW_MS), + Ok(()) + ); + } + + #[test] + fn should_not_let_a_master_key_stand_in_for_critical() { + // MASTER signs identity updates only, so it cannot replace the CRITICAL key. + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + ]); + assert!(check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS).is_err()); + } + + #[test] + fn should_allow_disabling_a_key_replaced_in_the_same_update() { + let replacement = authentication_key(4, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[replacement], &[1], NOW_MS), + Ok(()) + ); + let master_replacement = authentication_key(4, SecurityLevel::MASTER); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[master_replacement], &[0], NOW_MS), + Ok(()) + ); + } + + #[test] + fn should_allow_disabling_one_of_two_keys_of_a_level_but_not_both() { + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + authentication_key(2, SecurityLevel::CRITICAL), + ]); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS), + Ok(()) + ); + assert!(check_keys_can_be_disabled(&identity, &[], &[1, 2], NOW_MS).is_err()); + } + + #[test] + fn should_not_count_disabled_or_expired_keys_as_remaining() { + let mut disabled = authentication_key(2, SecurityLevel::CRITICAL); + disabled.set_disabled_at(NOW_MS - 1); + let expired = + authentication_key(3, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS)); + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + disabled, + expired, + ]); + assert!(check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS).is_err()); + + let expired_replacement = + authentication_key(4, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS)); + assert!( + check_keys_can_be_disabled(&identity, &[expired_replacement], &[1], NOW_MS).is_err() + ); + + let unexpired = + authentication_key(5, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS + 1)); + assert_eq!( + check_keys_can_be_disabled(&identity, &[unexpired], &[1], NOW_MS), + Ok(()) + ); + } + + #[test] + fn should_not_judge_keys_of_other_purposes_or_unknown_ids() { + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + key(1, Purpose::TRANSFER, SecurityLevel::CRITICAL), + key(2, Purpose::ENCRYPTION, SecurityLevel::MEDIUM), + ]); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1, 2, 9], NOW_MS), + Ok(()) + ); + // A non-authentication key does not keep an authentication level valid. + let identity_with_critical = { + let mut identity = identity; + identity.add_public_key(authentication_key(3, SecurityLevel::CRITICAL)); + identity + }; + assert!(check_keys_can_be_disabled(&identity_with_critical, &[], &[3], NOW_MS).is_err()); + } + + /// The refusal comes before the nonce is fetched: the mock answers no request, so a + /// refusal that reached the network would be a different error. + #[cfg(feature = "mocks")] + #[tokio::test] + async fn should_refuse_the_update_before_sending_anything() { + #[derive(Debug)] + struct NoSigner; + + #[async_trait::async_trait] + impl Signer for NoSigner { + async fn sign( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(dpp::ProtocolError::Generic("not signing".to_string())) + } + + async fn sign_create_witness( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(dpp::ProtocolError::Generic("not signing".to_string())) + } + + fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { + false + } + } + + let sdk = Sdk::new_mock(); + let result = one_key_per_level() + .update_identity_keys(&sdk, 0, vec![], vec![1], &NoSigner, None) + .await; + assert!( + matches!( + result, + Err(Error::LastAuthenticationKey(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + })) + ), + "{result:?}" + ); + } +} diff --git a/packages/wasm-sdk/src/error.rs b/packages/wasm-sdk/src/error.rs index 387b7750f7c..4bae134263a 100644 --- a/packages/wasm-sdk/src/error.rs +++ b/packages/wasm-sdk/src/error.rs @@ -284,6 +284,13 @@ impl From for WasmSdkError { retriable, ), EncryptedFor(e) => e.into(), + // A caller's request DIP-11 forbids: retrying cannot change it. + LastAuthenticationKey(e) => Self::new( + WasmSdkErrorKind::InvalidArgument, + e.to_string(), + None, + false, + ), error @ ProtocolVersionNotActive { .. } => { // The SDK's message names what is missing; a failed query for the network's // version is its source, and JS sees only the message. The refusal holds diff --git a/packages/wasm-sdk/src/state_transitions/identity.rs b/packages/wasm-sdk/src/state_transitions/identity.rs index 0fd35bb11ab..da20809ad7d 100644 --- a/packages/wasm-sdk/src/state_transitions/identity.rs +++ b/packages/wasm-sdk/src/state_transitions/identity.rs @@ -17,9 +17,9 @@ use dash_sdk::dpp::voting::vote_polls::VotePoll; use dash_sdk::dpp::voting::votes::resource_vote::v0::ResourceVoteV0; use dash_sdk::dpp::voting::votes::resource_vote::ResourceVote; use dash_sdk::dpp::voting::votes::Vote; -use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; use dash_sdk::platform::transition::put_identity::PutIdentity; use dash_sdk::platform::transition::top_up_identity::TopUpIdentity; +use dash_sdk::platform::transition::update_identity::UpdateIdentity; use dash_sdk::platform::transition::update_identity_key_limits::{ raised_key_limits, UpdateIdentityKeyLimits, }; @@ -585,7 +585,8 @@ impl WasmSdk { /// /// This method handles the complete update flow: /// 1. Validates the master key for signing - /// 2. Validates keys to add/disable + /// 2. Refuses to disable the last valid authentication key of a security level + /// (DIP-11); keys added in the same update count /// 3. Builds and signs the identity update transition /// 4. Broadcasts and waits for confirmation /// @@ -597,7 +598,7 @@ impl WasmSdk { options: IdentityUpdateOptionsJs, ) -> Result<(), WasmSdkError> { // Extract complex types first (borrows &options) - let mut identity: Identity = IdentityWasm::try_from_options(&options, "identity")?.into(); + let identity: Identity = IdentityWasm::try_from_options(&options, "identity")?.into(); let signer = IdentitySignerWasm::try_from_options(&options, "signer")?; let keys_to_add: Vec = if let Some(keys_array) = try_from_options_optional_with(&options, "addPublicKeys", |v| { @@ -635,12 +636,6 @@ impl WasmSdk { // Deserialize simple fields last (consumes options) let parsed = deserialize_identity_update_options(options.into())?; - // Increment the identity revision for the update transition - // The platform expects the new revision (current + 1) in the state transition - use dash_sdk::dpp::identity::accessors::IdentitySettersV0; - let original_revision = identity.revision(); - identity.set_revision(original_revision + 1); - // Find all valid master keys (AUTHENTICATION + MASTER + supported key type) let master_keys: Vec<_> = identity .public_keys() @@ -674,38 +669,20 @@ impl WasmSdk { // Get keys to disable let keys_to_disable = parsed.disable_public_keys.unwrap_or_default(); - // Get identity nonce - let identity_nonce = self - .inner_sdk() - .get_identity_nonce(identity.id(), true, settings) - .await - .map_err(|e| WasmSdkError::generic(format!("Failed to get identity nonce: {}", e)))?; - - // Create the identity update transition - use crate::settings::get_user_fee_increase; - use dash_sdk::dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; - use dash_sdk::dpp::state_transition::identity_update_transition::IdentityUpdateTransition; - - let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( - &identity, - &master_key_id, - keys_to_add, - keys_to_disable, - identity_nonce, - get_user_fee_increase(settings.as_ref()), - &signer, - self.inner_sdk().version(), - None, - ) - .await - .map_err(|e| WasmSdkError::with_context("Failed to create update transition", e))?; - - // Broadcast the transition - use dash_sdk::dpp::state_transition::proof_result::StateTransitionProofResult; - state_transition - .broadcast_and_wait::(self.inner_sdk(), settings) + // The SDK claims the next revision and refuses, before signing, an + // update that disables the last valid authentication key of a + // security level (DIP-11). + identity + .update_identity_keys( + self.inner_sdk(), + master_key_id, + keys_to_add, + keys_to_disable, + &signer, + settings, + ) .await - .map_err(|e| WasmSdkError::with_context("Failed to broadcast update", e))?; + .map_err(|e| WasmSdkError::with_context("Failed to update identity", e))?; Ok(()) } From 1e12c36274549b2b917733136ac4857dd1c4aa7e Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:10:28 +0000 Subject: [PATCH 02/15] fix(platform-wallet): keep received contact requests usable after a referenced key is disabled DIP-11 keeps disabled keys usable for verification, and a received contactRequest is immutable history that named the key while it was valid. The receive validators turned a since-disabled sender or recipient key into a permanent fault: the drain marked the channel broken (healable only by a fresh request from the contact) and the accept path refused, so a key rotation on either side cut the user off from their own payment history. The receive validators now record a disabled key as a warning. The send path uses the new `validate_contact_request_to_send`, which adds DIP-15's requirement that both keys of a request we mint are valid, so a disabled key there stays a hard error. No persisted state changes. Co-Authored-By: Claude Opus 5.5 --- .../src/wallet/identity/crypto/validation.rs | 185 ++++++++++++++---- .../identity/network/contact_requests.rs | 26 +-- 2 files changed, 166 insertions(+), 45 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs index 8b46ce08955..c315afa6d90 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -1,7 +1,9 @@ -//! Pre-send validation for DashPay contact requests. +//! Key validation for DashPay contact requests. //! -//! Validates that the sender and recipient identities have the correct key -//! types and purposes before a contact request is submitted to the platform. +//! [`validate_contact_request`] judges a request already on chain: liberal, +//! because the document is immutable. `validate_contact_request_to_send` +//! adds what DIP-15 asks of a request we are about to mint: both keys valid, +//! i.e. not disabled. use dash_sdk::platform::dashpay::{ recipient_key_purpose_is_acceptable_on_receive, sender_key_purpose_is_acceptable_on_receive, @@ -27,19 +29,20 @@ pub struct ContactRequestValidation { /// **permanently** broken — on-chain history demonstrably contains /// nonconforming-but-honest documents, and our acceptance policy (not the /// immutable request) is what might change. A purpose-only failure is a - /// non-permanent skip (log + retry next sweep); a key-TYPE / missing-key / - /// disabled-key failure stays permanent. + /// non-permanent skip (log + retry next sweep); a key-TYPE failure stays + /// permanent. /// /// **Read [`is_purpose_only`](Self::is_purpose_only), not this field, to /// decide skip-vs-break.** This flag alone is `true` even when a hard /// (non-purpose) error is *also* present; downgrading to a skip in that - /// case would mask a genuinely permanent failure (a disabled / wrong-type - /// key) into a retry-forever loop. + /// case would mask a genuinely permanent failure (a wrong-type key) into + /// a retry-forever loop. pub purpose_mismatch: bool, - /// `true` when at least one *non-purpose* hard error was recorded (missing - /// key, wrong key type, disabled key). Distinguishes "purpose mismatch is - /// the sole cause" (downgradable to a skip) from "purpose mismatch plus a - /// genuinely permanent fault" (must stay permanent). See + /// `true` when at least one *non-purpose* hard error was recorded (a + /// wrong key type, or a disabled key on the send path). Distinguishes + /// "purpose mismatch is the sole cause" (downgradable to a skip) from + /// "purpose mismatch plus a genuinely permanent fault" (must stay + /// permanent). See /// [`is_purpose_only`](Self::is_purpose_only). pub hard_error: bool, } @@ -96,8 +99,8 @@ impl ContactRequestValidation { /// Whether the *sole* cause of invalidity is a key-purpose mismatch — /// the only case that may be downgraded to a non-permanent skip. - /// A purpose mismatch that co-occurs with a hard error (disabled / - /// wrong-type key) is NOT purpose-only and must stay permanent. + /// A purpose mismatch that co-occurs with a hard error (a wrong-type + /// key) is NOT purpose-only and must stay permanent. pub fn is_purpose_only(&self) -> bool { self.purpose_mismatch && !self.hard_error } @@ -117,7 +120,9 @@ impl ContactRequestValidation { /// /// So an absent key is retryable, alongside a purpose mismatch. What stays /// permanent is what immutable facts make impossible: a key whose *type* - /// cannot do ECDH, and a key we have deliberately disabled. + /// cannot do ECDH. A disabled key is not among them: DIP-11 keeps disabled + /// keys usable for verification, and the request named the key while it + /// was valid, so the receive validators only warn about it. pub fn is_permanent(&self) -> bool { self.hard_error } @@ -145,8 +150,11 @@ impl ContactRequestValidation { /// validator is *liberal on receive* by necessity. Rejecting a document is not /// a retry, it is a permanent sentence on a contact relationship the user /// cannot renegotiate. What it keeps strict is what actually carries weight: -/// the ECDSA key-*type* gate (ECDH needs the full secp256k1 key) and the -/// disabled-key check. +/// the ECDSA key-*type* gate (ECDH needs the full secp256k1 key). A key +/// disabled since the request was sent is only a warning: DIP-11 keeps +/// disabled keys usable for verification, and DIP-15's "valid key" rule +/// binds the sender when minting, which +/// `validate_contact_request_to_send` enforces for our own requests. /// /// Three live cohorts are known: /// - **Newest** — bound `ENCRYPTION(sender)` / `DECRYPTION(recipient)`, our @@ -169,7 +177,7 @@ impl ContactRequestValidation { /// - Key type is `ECDSA_SECP256K1` (required for ECDH). /// - Key purpose is `ENCRYPTION` or `AUTHENTICATION` (bound or unbound) — /// anything else is flagged as a `purpose_mismatch` (non-permanent). -/// - Key is not disabled. +/// - A disabled key is a warning. /// /// **Recipient key (our key):** /// - Key at `recipient_key_index` exists on the recipient identity. @@ -177,7 +185,7 @@ impl ContactRequestValidation { /// - Key purpose is `DECRYPTION`, `ENCRYPTION`, `AUTHENTICATION` or /// `TRANSFER` — the node-operational purposes (`SYSTEM`, `VOTING`, /// `OWNER`) are flagged as a `purpose_mismatch`. -/// - Key is not disabled. +/// - A disabled key is a warning. /// /// The accepted sets live in `dash_sdk::platform::dashpay` as /// `*_key_purpose_is_acceptable_on_receive`, deliberately separate from the @@ -202,6 +210,39 @@ pub fn validate_contact_request( validation } +/// [`validate_contact_request`] for a request we are about to mint, plus +/// DIP-15's requirement that both key indices name a *valid* key: a disabled +/// key on either side is a hard error. Encrypting to a revoked key would hand +/// the payment xpub to whoever holds its private half. +pub(crate) fn validate_contact_request_to_send( + sender_identity: &Identity, + sender_key_index: u32, + recipient_identity: &Identity, + recipient_key_index: u32, +) -> ContactRequestValidation { + let mut validation = validate_contact_request( + sender_identity, + sender_key_index, + recipient_identity, + recipient_key_index, + ); + for (side, identity, key_index) in [ + ("Sender", sender_identity, sender_key_index), + ("Recipient", recipient_identity, recipient_key_index), + ] { + if let Some(disabled_at) = identity + .get_public_key_by_id(key_index) + .and_then(|key| key.disabled_at()) + { + validation.add_error(format!( + "{side} key {key_index} is disabled (at timestamp {disabled_at}); a new \ + contact request needs a valid key", + )); + } + } + validation +} + /// The sender half of [`validate_contact_request`] — the checks that need the /// **counterparty's** identity. /// @@ -240,10 +281,11 @@ pub(crate) fn validate_sender_key( )); } - // Must not be disabled. + // Disabled since the request was sent: still usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { - validation.add_error(format!( - "Sender key {} is disabled (at timestamp {})", + validation.add_warning(format!( + "Sender key {} is disabled (at timestamp {}); accepted, as the request \ + named it before", sender_key_index, disabled_at, )); } @@ -276,7 +318,7 @@ pub(crate) fn validate_sender_key( /// /// Deciding on this half alone is a real policy change, not just a reordering. /// When our key is purpose-rejected AND the sender's key carries a hard fault -/// (missing / disabled / wrong type), the composed [`validate_contact_request`] +/// (a key type that cannot do ECDH), the composed [`validate_contact_request`] /// would merge both, see `hard_error`, and mark the channel permanently /// broken. Stopping here classifies it purpose-only and leaves it queued. /// @@ -323,8 +365,8 @@ pub(crate) fn validate_recipient_key( // 0-2) or TRANSFER (key id 3) key. Purpose is not a security // boundary here — ECDH is defined over the secp256k1 keypair and // DIP-9 indexes the identity-key tree by type and id, never by - // purpose — so the type and disabled-key gates around this block - // are what actually protect the derivation. The node-operational + // purpose — so the type gate above this block is what actually + // protects the derivation. The node-operational // purposes (SYSTEM/VOTING/OWNER) stay out — nothing on chain // references them for DashPay — and remain a non-permanent purpose // mismatch: skip and retry, never break the channel, so a later @@ -338,10 +380,12 @@ pub(crate) fn validate_recipient_key( )); } - // Must not be disabled. + // Disabled since the request was sent: we still derive its + // scalar, so the request stays usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { - validation.add_error(format!( - "Recipient key {} is disabled (at timestamp {})", + validation.add_warning(format!( + "Recipient key {} is disabled (at timestamp {}); accepted, as the request \ + named it before", recipient_key_index, disabled_at, )); } @@ -531,11 +575,19 @@ mod tests { .any(|e| e.contains("not compatible with ECDH"))); } - #[test] - fn test_disabled_sender_key() { - let mut key = make_key(0, KeyType::ECDSA_SECP256K1, Purpose::ENCRYPTION); + fn disabled_key(id: u32, purpose: Purpose) -> IdentityPublicKey { + let mut key = make_key(id, KeyType::ECDSA_SECP256K1, purpose); key.set_disabled_at(12345); - let sender = make_identity(vec![key]); + key + } + + /// DIP-11: disabled keys "can be used for signature verification". A + /// received request is immutable history that named a key valid when it + /// was sent; the sender disabling that key since must not lock us out of + /// the channel. It is a warning, never a failure. + #[test] + fn should_accept_a_received_request_naming_a_since_disabled_sender_key() { + let sender = make_identity(vec![disabled_key(0, Purpose::ENCRYPTION)]); let recipient = make_identity(vec![make_key( 0, KeyType::ECDSA_SECP256K1, @@ -543,8 +595,75 @@ mod tests { )]); let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!(result.errors.iter().any(|e| e.contains("disabled"))); + assert!(result.is_valid, "errors: {:?}", result.errors); + assert!(!result.is_permanent()); + assert!(result.warnings.iter().any(|w| w.contains("disabled"))); + + let sender_half = validate_sender_key(&sender, 0); + assert!(sender_half.is_valid, "errors: {:?}", sender_half.errors); + } + + /// Our own key disabled after the request arrived: we still derive its + /// scalar, so the request stays usable. + #[test] + fn should_accept_a_received_request_naming_our_since_disabled_recipient_key() { + let sender = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + Purpose::ENCRYPTION, + )]); + let recipient = make_identity(vec![disabled_key(0, Purpose::DECRYPTION)]); + + let result = validate_contact_request(&sender, 0, &recipient, 0); + assert!(result.is_valid, "errors: {:?}", result.errors); + assert!(result.warnings.iter().any(|w| w.contains("disabled"))); + + let recipient_half = validate_recipient_key(&recipient, 0); + assert!( + recipient_half.is_valid, + "errors: {:?}", + recipient_half.errors + ); + assert!(!recipient_half.is_permanent()); + } + + /// DIP-15 requires a valid key when SENDING: neither side may be disabled. + #[test] + fn should_refuse_to_send_with_a_disabled_key() { + let enabled_sender = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + Purpose::ENCRYPTION, + )]); + let enabled_recipient = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + Purpose::DECRYPTION, + )]); + let disabled_sender = make_identity(vec![disabled_key(0, Purpose::ENCRYPTION)]); + let disabled_recipient = make_identity(vec![disabled_key(0, Purpose::DECRYPTION)]); + + let valid = validate_contact_request_to_send(&enabled_sender, 0, &enabled_recipient, 0); + assert!(valid.is_valid, "errors: {:?}", valid.errors); + + for (sender, recipient, side) in [ + (&disabled_sender, &enabled_recipient, "Sender"), + (&enabled_sender, &disabled_recipient, "Recipient"), + ] { + let result = validate_contact_request_to_send(sender, 0, recipient, 0); + assert!( + !result.is_valid, + "a disabled {side} key must refuse the send" + ); + assert!( + result + .errors + .iter() + .any(|e| e.starts_with(side) && e.contains("disabled")), + "errors: {:?}", + result.errors + ); + } } #[test] diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index 4d9e3e2eafb..ad52d7ff622 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -18,6 +18,7 @@ use super::sdk_writer::SendContactRequestParams; use super::*; use crate::broadcaster::TransactionBroadcaster; use crate::error::PlatformWalletError; +use crate::wallet::identity::crypto::validation::validate_contact_request_to_send; use crate::wallet::identity::types::dashpay::contact_request::ContactRequest; use crate::wallet::identity::types::dashpay::established_contact::EstablishedContact; use crate::wallet::identity::ManagedIdentity; @@ -474,14 +475,14 @@ impl DashPayView<'_, B> { let recipient_key_index = select_recipient_key_index(&recipient_identity)?; - // 3b. Gate the selected key pair through the same validator - // the receive/accept paths use, BEFORE any ECDH or - // broadcast. The selectors above pick plausible indices; - // the validator pins the full contract (key types, not - // disabled, purpose policy) so a malformed identity can't - // reach the encrypt-and-broadcast stage with a key that - // would poison the channel. - let validation = crate::wallet::identity::crypto::validation::validate_contact_request( + // 3b. Gate the selected key pair through the receive validator + // plus DIP-15's send-side rule (neither key disabled), BEFORE + // any ECDH or broadcast. The selectors above pick plausible + // indices; the validator pins the full contract (key types, + // not disabled) so a malformed identity can't reach the + // encrypt-and-broadcast stage with a key that would poison + // the channel. + let validation = validate_contact_request_to_send( &sender_identity, sender_key_index, &recipient_identity, @@ -1208,7 +1209,7 @@ fn select_recipient_key_index(recipient_identity: &Identity) -> Result = recipient_identity .public_keys() .iter() @@ -1236,7 +1237,7 @@ fn select_recipient_key_index(recipient_identity: &Identity) -> Result DashPayView<'_, B> { /// a superseding request from the CONTACT to heal, an appeal the user /// cannot file. /// - Only a fault that immutable facts make permanent — a key type that - /// cannot do ECDH, a key we disabled — breaks the channel, so the sweep - /// stops collecting it. + /// cannot do ECDH — breaks the channel, so the sweep stops collecting + /// it. A key disabled since the request was sent is not one: DIP-11 + /// keeps it usable, so the validators only warn about it. /// /// Returns `true` when the caller should clear the entry from the queue. /// From 65668db673b0ae6ef6938f2c8a90fc150eeee072 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:17:01 +0000 Subject: [PATCH 03/15] fix: stop refusing received contact requests for the purpose of their keys DIP-15 constrains a contact request's sender and recipient keys by type only: they must allow a Diffie-Hellman key agreement (secp256k1). It says nothing about purpose. The receive validators still rejected a sender key that was not ENCRYPTION/AUTHENTICATION and a recipient SYSTEM/VOTING/OWNER key, leaving such requests queued and never decrypted. Remove `sender_key_purpose_is_acceptable_on_receive` and `recipient_key_purpose_is_acceptable_on_receive` from `dash_sdk::platform::dashpay`, and the purpose-mismatch machinery (`purpose_mismatch`, `add_purpose_error`, `is_purpose_only`) from platform-wallet's `ContactRequestValidation`, which nothing sets any more. The key-type gate stays, and so do the mint-side predicates and selectors: what we create still references ENCRYPTION/DECRYPTION keys only. The drain's retry summary now names what it counts (a referenced key not found yet); the recipient-first drain test pins that case instead of the retired purpose rejection. Co-Authored-By: Claude Opus 5.5 --- .../src/wallet/identity/crypto/validation.rs | 343 +++++------------- .../identity/network/contact_requests.rs | 81 ++--- .../src/wallet/identity/network/payments.rs | 27 +- .../src/platform/dashpay/contact_request.rs | 101 +----- packages/rs-sdk/src/platform/dashpay/mod.rs | 5 +- 5 files changed, 146 insertions(+), 411 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs index c315afa6d90..60f16aa913d 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -5,9 +5,6 @@ //! adds what DIP-15 asks of a request we are about to mint: both keys valid, //! i.e. not disabled. -use dash_sdk::platform::dashpay::{ - recipient_key_purpose_is_acceptable_on_receive, sender_key_purpose_is_acceptable_on_receive, -}; use dpp::identity::accessors::IdentityGettersV0; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dpp::identity::{Identity, KeyType}; @@ -21,29 +18,9 @@ pub struct ContactRequestValidation { pub errors: Vec, /// Non-fatal warnings the caller may want to surface. pub warnings: Vec, - /// `true` when a key-PURPOSE mismatch was seen (e.g. a legacy 2024 doc - /// referencing an AUTHENTICATION key). - /// - /// This classification is load-bearing for the sync sweep / accept paths: - /// a purpose mismatch must NOT mark the payment channel - /// **permanently** broken — on-chain history demonstrably contains - /// nonconforming-but-honest documents, and our acceptance policy (not the - /// immutable request) is what might change. A purpose-only failure is a - /// non-permanent skip (log + retry next sweep); a key-TYPE failure stays - /// permanent. - /// - /// **Read [`is_purpose_only`](Self::is_purpose_only), not this field, to - /// decide skip-vs-break.** This flag alone is `true` even when a hard - /// (non-purpose) error is *also* present; downgrading to a skip in that - /// case would mask a genuinely permanent failure (a wrong-type key) into - /// a retry-forever loop. - pub purpose_mismatch: bool, - /// `true` when at least one *non-purpose* hard error was recorded (a - /// wrong key type, or a disabled key on the send path). Distinguishes - /// "purpose mismatch is the sole cause" (downgradable to a skip) from - /// "purpose mismatch plus a genuinely permanent fault" (must stay - /// permanent). See - /// [`is_purpose_only`](Self::is_purpose_only). + /// `true` when at least one permanent error was recorded (a wrong key + /// type, or a disabled key on the send path). See + /// [`is_permanent`](Self::is_permanent). pub hard_error: bool, } @@ -53,7 +30,6 @@ impl Default for ContactRequestValidation { is_valid: true, errors: Vec::new(), warnings: Vec::new(), - purpose_mismatch: false, hard_error: false, } } @@ -65,9 +41,8 @@ impl ContactRequestValidation { Self::default() } - /// Add a hard (non-purpose) error: sets `is_valid = false` AND flags - /// `hard_error` so a co-occurring purpose mismatch can't downgrade this - /// genuinely-permanent fault to a skip. + /// Add a hard (permanent) error: sets `is_valid = false` AND flags + /// `hard_error`. pub fn add_error(&mut self, error: String) { self.errors.push(error); self.is_valid = false; @@ -82,29 +57,11 @@ impl ContactRequestValidation { self.is_valid = false; } - /// Add a key-PURPOSE error: sets `is_valid = false` AND flags - /// `purpose_mismatch` so callers can downgrade a *purpose-only* failure - /// to a non-permanent skip rather than a permanent broken-channel mark. - /// Does NOT set `hard_error`. - pub fn add_purpose_error(&mut self, error: String) { - self.errors.push(error); - self.is_valid = false; - self.purpose_mismatch = true; - } - /// Add a non-fatal warning. pub fn add_warning(&mut self, warning: String) { self.warnings.push(warning); } - /// Whether the *sole* cause of invalidity is a key-purpose mismatch — - /// the only case that may be downgraded to a non-permanent skip. - /// A purpose mismatch that co-occurs with a hard error (a wrong-type - /// key) is NOT purpose-only and must stay permanent. - pub fn is_purpose_only(&self) -> bool { - self.purpose_mismatch && !self.hard_error - } - /// Whether this failure can never resolve on its own — the only kind that /// may permanently break a contact's payment channel. /// @@ -118,7 +75,7 @@ impl ContactRequestValidation { /// the user cannot repair — only a fresh request from the CONTACT clears /// the flag. /// - /// So an absent key is retryable, alongside a purpose mismatch. What stays + /// So an absent key is retryable. What stays /// permanent is what immutable facts make impossible: a key whose *type* /// cannot do ECDH. A disabled key is not among them: DIP-11 keeps disabled /// keys usable for verification, and the request named the key while it @@ -134,9 +91,6 @@ impl ContactRequestValidation { if !other.is_valid { self.is_valid = false; } - if other.purpose_mismatch { - self.purpose_mismatch = true; - } if other.hard_error { self.hard_error = true; } @@ -175,27 +129,18 @@ impl ContactRequestValidation { /// **Sender key:** /// - Key at `sender_key_index` exists on the sender identity. /// - Key type is `ECDSA_SECP256K1` (required for ECDH). -/// - Key purpose is `ENCRYPTION` or `AUTHENTICATION` (bound or unbound) — -/// anything else is flagged as a `purpose_mismatch` (non-permanent). /// - A disabled key is a warning. /// /// **Recipient key (our key):** /// - Key at `recipient_key_index` exists on the recipient identity. /// - Key type is compatible (`ECDSA_SECP256K1` or `ECDSA_HASH160`). -/// - Key purpose is `DECRYPTION`, `ENCRYPTION`, `AUTHENTICATION` or -/// `TRANSFER` — the node-operational purposes (`SYSTEM`, `VOTING`, -/// `OWNER`) are flagged as a `purpose_mismatch`. /// - A disabled key is a warning. /// -/// The accepted sets live in `dash_sdk::platform::dashpay` as -/// `*_key_purpose_is_acceptable_on_receive`, deliberately separate from the -/// stricter `recipient_key_purpose_is_valid` that governs the requests we -/// *mint*: accepting history is not the same decision as choosing a key for a -/// new document, and only the latter can still practice key separation. -/// -/// A failure whose *only* cause is a purpose mismatch sets -/// [`ContactRequestValidation::purpose_mismatch`], signalling callers to skip -/// (and retry) rather than permanently break the channel. +/// Key purpose is not checked: DIP-15 constrains these keys by type only, and +/// ECDH is defined over the secp256k1 keypair whatever its purpose. Purpose +/// policy belongs to the requests we *mint* +/// (`dash_sdk::platform::dashpay::recipient_key_purpose_is_valid` and the +/// own-key selector), where key separation can still be practiced. pub fn validate_contact_request( sender_identity: &Identity, sender_key_index: u32, @@ -267,20 +212,6 @@ pub(crate) fn validate_sender_key( )); } - // ENCRYPTION is the modern convention; legacy dashj documents - // reference an AUTHENTICATION key. Both are accepted on receive — - // the document is immutable, so rejecting it is a permanent - // sentence on a contact the user cannot appeal. Anything else is - // still a non-permanent purpose mismatch (skip and retry). - if !sender_key_purpose_is_acceptable_on_receive(key.purpose()) { - validation.add_purpose_error(format!( - "Sender key {} has purpose {:?}, but ENCRYPTION or AUTHENTICATION is \ - required for contact requests", - sender_key_index, - key.purpose(), - )); - } - // Disabled since the request was sent: still usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { validation.add_warning(format!( @@ -307,25 +238,25 @@ pub(crate) fn validate_sender_key( /// /// Split out because the deferred-crypto drain would otherwise pay a Platform /// round trip (`Identity::fetch` of the contact) before it could discover that -/// the request is unusable for a reason it could have known locally. A -/// purpose-rejected entry stays queued by design — the policy, not the -/// immutable document, is what might change — so that fetch was repeating on -/// every sweep, forever. Mainnet logs from one wallet show 27 contacts and 396 -/// such fetch-then-reject cycles in a single session. Running this half first +/// the request is unusable for a reason it could have known locally. An entry +/// naming a key our identity does not have yet stays queued by design — the +/// identity can gain it — so that fetch would repeat on every sweep. (Mainnet +/// logs from one wallet once showed 27 contacts and 396 such +/// fetch-then-reject cycles in a single session.) Running this half first /// costs nothing and removes the network entirely from that loop. /// /// # What this changes for a MIXED failure /// /// Deciding on this half alone is a real policy change, not just a reordering. -/// When our key is purpose-rejected AND the sender's key carries a hard fault -/// (a key type that cannot do ECDH), the composed [`validate_contact_request`] -/// would merge both, see `hard_error`, and mark the channel permanently -/// broken. Stopping here classifies it purpose-only and leaves it queued. +/// When our key is absent AND the sender's key carries a hard fault (a key +/// type that cannot do ECDH), the composed [`validate_contact_request`] would +/// merge both, see `hard_error`, and mark the channel permanently broken. +/// Stopping here classifies it retryable and leaves it queued. /// /// That is the intended outcome. The `hard_error` precedence exists to stop a /// genuinely permanent fault from becoming a retry-forever loop — but the /// forever-loop it guards against was expensive precisely because each retry -/// fetched. With the fetch gone, a purpose-rejected entry costs a map lookup +/// fetched. With the fetch gone, a retryable entry costs a map lookup /// per sweep, while marking the channel broken is unappealable by the user: /// only a fresh request from the CONTACT clears it. Deferring the broken mark /// until the fault is one we can see locally trades a cheap retry for an @@ -359,27 +290,6 @@ pub(crate) fn validate_recipient_key( } } - // Four honest cohorts reach this point: the newest references our - // DECRYPTION key, the mobile population our ENCRYPTION key, and - // the legacy Android/dashj population our AUTHENTICATION (key ids - // 0-2) or TRANSFER (key id 3) key. Purpose is not a security - // boundary here — ECDH is defined over the secp256k1 keypair and - // DIP-9 indexes the identity-key tree by type and id, never by - // purpose — so the type gate above this block is what actually - // protects the derivation. The node-operational - // purposes (SYSTEM/VOTING/OWNER) stay out — nothing on chain - // references them for DashPay — and remain a non-permanent purpose - // mismatch: skip and retry, never break the channel, so a later - // evidence-driven widening can still pick those contacts up. - if !recipient_key_purpose_is_acceptable_on_receive(key.purpose()) { - validation.add_purpose_error(format!( - "Recipient key {} has purpose {:?}, which is not accepted for contact \ - requests", - recipient_key_index, - key.purpose(), - )); - } - // Disabled since the request was sent: we still derive its // scalar, so the request stays usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { @@ -523,24 +433,6 @@ mod tests { assert!(result.errors.iter().any(|e| e.contains("ECDSA_SECP256K1"))); } - #[test] - fn test_sender_wrong_purpose() { - // VOTING, not AUTHENTICATION: the legacy dashj cohort pairs an - // AUTHENTICATION sender key with an AUTHENTICATION recipient key and - // is accepted on receive, so AUTHENTICATION does not exercise - // the sender-side rejection this test is about. - let sender = make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, Purpose::VOTING)]); - let recipient = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::DECRYPTION, - )]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!(result.errors.iter().any(|e| e.contains("ENCRYPTION"))); - } - #[test] fn test_recipient_key_missing() { let sender = make_identity(vec![make_key( @@ -688,19 +580,14 @@ mod tests { // newest cohort uses bound ENCRYPTION(sender)/DECRYPTION(recipient). // Consensus enforces neither purpose nor boundedness, and mainnet adds a // third, legacy Android/dashj cohort referencing AUTHENTICATION/TRANSFER - // keys. So the validator accepts ENCRYPTION-or-AUTHENTICATION for the - // sender and everything but the node-operational purposes for the - // recipient, while keeping the - // ECDSA type gate and the disabled-key check — those are the checks that - // actually protect the ECDH. + // keys. DIP-15 constrains these keys by type only, so the validator + // accepts every purpose on receive and keeps the ECDSA type gate — the + // check that actually protects the ECDH. // ----------------------------------------------------------------------- /// Mobile-cohort shape: sender references an ENCRYPTION key, recipient /// (our key) is ALSO an ENCRYPTION key (mobile identities have no - /// DECRYPTION key). This must pass. The companion AUTHENTICATION test - /// below pins the recipient-purpose gate: without that gate an - /// AUTHENTICATION recipient key is silently accepted (it "passes" for - /// the wrong reason). + /// DECRYPTION key). This must pass. #[test] fn mobile_cohort_recipient_encryption_key_is_accepted() { let sender = make_identity(vec![make_key( @@ -720,7 +607,6 @@ mod tests { "mobile-cohort ENC/ENC request must validate, errors: {:?}", result.errors ); - assert!(!result.purpose_mismatch); } /// Legacy Android/dashj shape: the inbound request references OUR @@ -750,7 +636,6 @@ mod tests { document, errors: {:?}", result.errors ); - assert!(!result.purpose_mismatch); } } @@ -771,7 +656,6 @@ mod tests { let result = validate_contact_request(&sender, 1, &recipient, 1); assert!(result.is_valid, "errors: {:?}", result.errors); - assert!(!result.purpose_mismatch); } /// A key id the identity does not have **yet** must not be permanent. @@ -819,34 +703,77 @@ mod tests { ); } - /// The node-operational purposes are the ones still refused for a - /// recipient key — and they must stay a non-permanent purpose mismatch, so - /// a future evidence-driven widening can still pick those contacts up - /// instead of finding them broken. + const ALL_PURPOSES: [Purpose; 7] = [ + Purpose::AUTHENTICATION, + Purpose::ENCRYPTION, + Purpose::DECRYPTION, + Purpose::TRANSFER, + Purpose::SYSTEM, + Purpose::VOTING, + Purpose::OWNER, + ]; + + /// DIP-15 constrains the keys of a contact request by type only (ECDH + /// needs secp256k1), never by purpose, so a received request must not be + /// refused for the purpose of either key. #[test] - fn recipient_node_operational_key_is_rejected_as_purpose_mismatch() { - let sender = make_identity(vec![make_key( + fn should_accept_a_received_request_whatever_the_key_purposes() { + for sender_purpose in ALL_PURPOSES { + for recipient_purpose in ALL_PURPOSES { + let sender = + make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, sender_purpose)]); + let recipient = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + recipient_purpose, + )]); + + let result = validate_contact_request(&sender, 0, &recipient, 0); + assert!( + result.is_valid, + "{sender_purpose:?} sender / {recipient_purpose:?} recipient: {:?}", + result.errors + ); + } + } + } + + /// The type gate is the genuine one and stays: a key that cannot do + /// secp256k1 ECDH fails permanently, whatever its purpose. + #[test] + fn should_still_refuse_a_key_type_that_cannot_do_ecdh() { + let enabled = make_identity(vec![make_key( 0, KeyType::ECDSA_SECP256K1, Purpose::ENCRYPTION, )]); - for purpose in [Purpose::SYSTEM, Purpose::VOTING, Purpose::OWNER] { - let recipient = make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, purpose)]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!( - !result.is_valid, - "a {purpose:?} recipient key must be rejected" - ); - assert!( - result.purpose_mismatch && !result.hard_error, - "{purpose:?} must be a PURPOSE mismatch (non-permanent skip), not a hard failure" - ); + for purpose in ALL_PURPOSES { + let bls = make_identity(vec![make_key(0, KeyType::BLS12_381, purpose)]); + for result in [ + validate_contact_request(&bls, 0, &enabled, 0), + validate_contact_request(&enabled, 0, &bls, 0), + ] { + assert!(!result.is_valid, "{purpose:?}"); + assert!(result.is_permanent(), "{purpose:?}"); + } } } + /// `merge` must carry the `hard_error` flag so a hard fault in a merged + /// sub-result can't be lost. + #[test] + fn merge_propagates_hard_error() { + let mut a = ContactRequestValidation::new(); + a.add_absent_key_error("absent".into()); + let mut b = ContactRequestValidation::new(); + b.add_error("hard".into()); + a.merge(b); + assert!(!a.is_valid); + assert!(a.is_permanent()); + } + /// Sender ENCRYPTION + recipient DECRYPTION (our existing convention, - /// the newest 2026 cohort) still validates and is not a purpose mismatch. + /// the newest 2026 cohort) still validates. #[test] fn bound_convention_enc_dec_still_validates() { let sender = make_identity(vec![make_key( @@ -862,102 +789,6 @@ mod tests { let result = validate_contact_request(&sender, 4, &recipient, 5); assert!(result.is_valid, "errors: {:?}", result.errors); - assert!(!result.purpose_mismatch); - } - - /// A sender purpose outside the accepted set stays a purpose mismatch — - /// the classification flag must be set so the sweep/accept paths skip - /// rather than permanently break the channel. TRANSFER stands in for - /// AUTHENTICATION here: the latter is now an accepted legacy shape, but - /// no observed document puts TRANSFER on the sender side. - #[test] - fn unaccepted_sender_purpose_is_a_purpose_mismatch() { - let sender = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::TRANSFER, - )]); - let recipient = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::DECRYPTION, - )]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!( - result.purpose_mismatch, - "a sender purpose mismatch must be flagged so the channel is not permanently broken" - ); - } - - /// A NON-purpose failure (wrong key type) must NOT set `purpose_mismatch` - /// — it stays a hard/permanent failure that breaks the channel. - #[test] - fn wrong_key_type_is_not_a_purpose_mismatch() { - let sender = make_identity(vec![make_key(0, KeyType::BLS12_381, Purpose::ENCRYPTION)]); - let recipient = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::DECRYPTION, - )]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!( - !result.purpose_mismatch, - "a key-TYPE failure is permanent, not a purpose mismatch" - ); - } - - /// **#5 — a purpose mismatch that co-occurs with a hard error must NOT be - /// downgraded to a skip.** `add_purpose_error` flags `purpose_mismatch` - /// even when a genuinely-permanent hard error (disabled / missing / - /// wrong-type key) is also present; reading the bare flag to decide - /// skip-vs-break would mask that permanent fault into a retry-forever - /// loop. `is_purpose_only()` is the correct gate. - #[test] - fn purpose_mismatch_with_hard_error_is_not_purpose_only() { - let mut v = ContactRequestValidation::new(); - v.add_purpose_error("recipient key purpose is AUTHENTICATION".into()); - v.add_error("sender key is disabled".into()); - - assert!(!v.is_valid); - assert!(v.purpose_mismatch, "the purpose flag is still raised"); - assert!( - !v.is_purpose_only(), - "a purpose mismatch alongside a hard error is NOT purpose-only — must stay permanent" - ); - } - - /// A lone purpose mismatch IS purpose-only → skippable. - #[test] - fn lone_purpose_mismatch_is_purpose_only() { - let mut v = ContactRequestValidation::new(); - v.add_purpose_error("recipient key purpose is AUTHENTICATION".into()); - assert!(v.is_purpose_only()); - } - - /// A lone hard error is never purpose-only. - #[test] - fn lone_hard_error_is_not_purpose_only() { - let mut v = ContactRequestValidation::new(); - v.add_error("sender key is disabled".into()); - assert!(!v.is_purpose_only()); - } - - /// `merge` must carry the `hard_error` flag so a hard fault in a merged - /// sub-result can't be lost (which would re-open the masking bug). - #[test] - fn merge_propagates_hard_error() { - let mut a = ContactRequestValidation::new(); - a.add_purpose_error("purpose".into()); - let mut b = ContactRequestValidation::new(); - b.add_error("hard".into()); - a.merge(b); - assert!(a.purpose_mismatch); - assert!(a.hard_error); - assert!(!a.is_purpose_only()); } // ----------------------------------------------------------------------- diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index ad52d7ff622..c4c27b31a0b 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -1193,9 +1193,8 @@ pub(super) fn external_account_needs_rebuild( /// /// That mainnet's legacy Android/dashj population *does* reference /// AUTHENTICATION/TRANSFER keys is a fact about immutable history, handled by -/// the wider receive-side policy -/// ([`dash_sdk::platform::dashpay::recipient_key_purpose_is_acceptable_on_receive`]). -/// It must not relax what we mint: this selector calls +/// the receive side, which does not judge purpose at all (DIP-15 constrains +/// these keys by type only). It must not relax what we mint: this selector calls /// [`dash_sdk::platform::dashpay::recipient_key_purpose_is_valid`] for /// membership, so the cohort cannot drift from the SDK's request-creation /// gate. Only the preference ORDER below (DECRYPTION first, ENCRYPTION second) @@ -2224,8 +2223,8 @@ impl DashPayView<'_, B> { // knowing that. Reading it back off an exported log beats asking the // user to query Platform. On-chain public metadata only; no key data. // Gated on the level: the block allocates a set, a string per key and a - // join, and takes the wallet-manager read lock. Purpose-rejected - // entries stay queued and revisit this path every sweep, so leaving + // join, and takes the wallet-manager read lock. Retryable entries + // stay queued and revisit this path every sweep, so leaving // that work unconditional would add recurring cost to the very path // this change exists to make cheap. if tracing::enabled!(tracing::Level::INFO) { @@ -2272,14 +2271,14 @@ impl DashPayView<'_, B> { } let mut cleared: Vec = Vec::new(); - // Distinct key-purpose rejections seen this drain, and how many entries - // each blocked — summarised once at the end instead of one WARN per - // entry. A purpose-rejected entry stays queued by design (the policy, - // not the immutable document, is what might change), so per-entry - // WARNing repeats every sweep for the life of the wallet: mainnet logs - // from one wallet show 396 such lines for 27 contacts in a single - // session. Every reason still reaches the log, once, with its count. - let mut policy_blocked: std::collections::BTreeMap = + // Distinct retryable rejections (a referenced key we do not have yet) + // seen this drain, and how many entries each blocked — summarised once + // at the end instead of one WARN per entry. Such an entry stays queued + // by design (the identity can gain the key), so per-entry WARNing + // would repeat every sweep: mainnet logs from one wallet once showed + // 396 such lines for 27 contacts in a single session. Every reason + // still reaches the log, once, with its count. + let mut retry_blocked: std::collections::BTreeMap = std::collections::BTreeMap::new(); // How much of `cleared` is already dequeued + persisted, and the running // total actually removed. Bookkeeping lands per entry, so at most one @@ -2399,13 +2398,12 @@ impl DashPayView<'_, B> { // Validate OUR key first — it needs nothing but the // resident identity, so a request that can never be used is // rejected before spending a Platform round trip on the - // contact. This is the dominant rejection in practice - // (legacy documents reference our AUTHENTICATION/TRANSFER - // key), and because such an entry stays queued the fetch - // below would otherwise repeat on every sweep, forever. + // contact. A request naming a key we do not have yet stays + // queued, so the fetch below would otherwise repeat on + // every sweep. // - // Deciding here means a MIXED failure — our key - // purpose-rejected and the contact's key hard-faulted — + // Deciding here means a MIXED failure — our key absent + // and the contact's key hard-faulted — // leaves the entry queued where a composed validator would // mark the channel broken. Deliberate: see // `validate_recipient_key`. Marking broken is unappealable @@ -2426,9 +2424,8 @@ impl DashPayView<'_, B> { ); continue; }; - // Did only the widened receive-side policy let this request - // through — i.e. does either referenced key name a purpose - // we would never mint ourselves? That marks it as the + // Does either referenced key name a purpose we would never + // mint ourselves? That marks it as the // legacy dashj cohort, whose ECDH/AES byte compatibility // with our implementation has not been cross-validated // against a dashj-produced payload. Used far below to keep @@ -2454,7 +2451,7 @@ impl DashPayView<'_, B> { .apply_drain_validation_failure( entry, recipient_validation, - &mut policy_blocked, + &mut retry_blocked, ) .await { @@ -2496,11 +2493,10 @@ impl DashPayView<'_, B> { } }; - // The sender side of the legacy classification — the - // widening moved the sender rule from ENCRYPTION-only to - // ENCRYPTION-or-AUTHENTICATION too, so an AUTHENTICATION - // sender against a mint-valid recipient is just as much an - // unverified legacy payload. + // The sender side of the legacy classification — we mint + // ENCRYPTION senders only, so a sender key of any other + // purpose against a mint-valid recipient is just as much + // an unverified legacy payload. let accepted_by_legacy_widening = recipient_widened || contact_identity .get_public_key_by_id(*contact_encryption_key_index) @@ -2516,7 +2512,7 @@ impl DashPayView<'_, B> { ); if !validation.is_valid { if self - .apply_drain_validation_failure(entry, validation, &mut policy_blocked) + .apply_drain_validation_failure(entry, validation, &mut retry_blocked) .await { cleared.push(entry.key()); @@ -2751,19 +2747,19 @@ impl DashPayView<'_, B> { .flush_drained_contact_crypto(&entries, &cleared[flushed..]) .await; - // One line for every entry the key-purpose policy turned away, instead - // of one per entry per sweep. Kept at WARN and carrying the distinct + // One line for every entry left queued as retryable, instead of one + // per entry per sweep. Kept at WARN and carrying the distinct // reasons: this is the signal that a live on-chain cohort is failing // our acceptance policy, which is exactly how the legacy dashj cohort // was found — it must stay visible in an exported log, just not 27 // times a pass. - if !policy_blocked.is_empty() { - let blocked: usize = policy_blocked.values().sum(); + if !retry_blocked.is_empty() { + let blocked: usize = retry_blocked.values().sum(); tracing::warn!( entries = blocked, - reasons = ?policy_blocked, - "drain: contact requests left queued by the key-purpose policy \ - (not marking broken; they retry when the policy changes)" + reasons = ?retry_blocked, + "drain: contact requests left queued on a retryable key-index failure \ + (not marking broken; they retry every sweep)" ); } // One-line verdict for the pass. "Did the legacy contacts build?" is @@ -2782,9 +2778,8 @@ impl DashPayView<'_, B> { /// The drain's validation-failure policy, shared by the recipient-half and /// sender-half checks so both halves classify identically. /// - /// - A failure that can still resolve — a purpose mismatch (our acceptance - /// policy might change) or an absent key id (identities gain keys) — is - /// counted into `policy_blocked` and left queued. The `contactRequest` + /// - A failure that can still resolve — an absent key id (identities gain + /// keys) — is counted into `retry_blocked` and left queued. The `contactRequest` /// cleared consensus and is immutable; a channel marked broken here needs /// a superseding request from the CONTACT to heal, an appeal the user /// cannot file. @@ -2795,7 +2790,7 @@ impl DashPayView<'_, B> { /// /// Returns `true` when the caller should clear the entry from the queue. /// - /// Takes `validation` by value: a purpose-rejected entry stays queued by + /// Takes `validation` by value: a retryable entry stays queued by /// design and comes back through here on every sweep, so cloning its /// reasons into the summary would allocate once per contact per pass for /// the life of the wallet. Moving them costs nothing — neither caller uses @@ -2804,16 +2799,16 @@ impl DashPayView<'_, B> { &self, entry: &crate::changeset::PendingContactCrypto, validation: crate::wallet::identity::crypto::validation::ContactRequestValidation, - policy_blocked: &mut std::collections::BTreeMap, + retry_blocked: &mut std::collections::BTreeMap, ) -> bool { if !validation.is_permanent() { tracing::debug!( owner = %entry.owner_identity_id, contact = %entry.contact_id, errors = ?validation.errors, - "drain: contact request key-purpose mismatch; leaving queued (not marking broken)" + "drain: contact request names a key not found yet; leaving queued (not marking broken)" ); for reason in validation.errors { - *policy_blocked.entry(reason).or_default() += 1; + *retry_blocked.entry(reason).or_default() += 1; } return false; } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index f6a4f3919b1..0330ebfd6ca 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -11103,8 +11103,9 @@ mod tests { ); } - /// An unaccepted recipient PURPOSE — the actual repeating case — is decided - /// locally, and a co-occurring sender-side hard fault does not change that. + /// A request naming a key of ours we do not have yet — the repeating case + /// — is decided locally, and a co-occurring sender-side hard fault does + /// not change that. /// /// This is the discriminating test for both halves of the change: /// @@ -11114,13 +11115,13 @@ mod tests { /// mark the channel broken and clear the entry. Asserting the entry is /// still queued and the channel still intact therefore proves the fetch /// never happened; a "hard faults only" short-circuit that still fetched - /// for purpose mismatches would fail here. - /// * It pins the deliberate mixed-failure policy change: purpose-rejected - /// on our side wins, and the entry stays recoverable. + /// for retryable failures would fail here. + /// * It pins the deliberate mixed-failure policy: a retryable failure on + /// our side wins, and the entry stays recoverable. /// /// Drained twice, because the cost avoided is per sweep, not once. #[tokio::test] - async fn unaccepted_recipient_purpose_never_fetches_and_stays_recoverable() { + async fn absent_recipient_key_never_fetches_and_stays_recoverable() { use crate::changeset::{PendingContactCrypto, PendingContactCryptoOp}; use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; @@ -11129,11 +11130,11 @@ mod tests { let owner = Identifier::from([0xAA; 32]); let contact = Identifier::from([0xBB; 32]); - // Our key at the referenced index: valid ECDSA, but a purpose the - // receive-side policy does not accept — a purpose-only rejection. + // Our identity holds key 0 only; the request names our key 1, which + // we do not have yet — a retryable rejection. let our_key = IdentityPublicKey::V0(IdentityPublicKeyV0 { id: 0, - purpose: Purpose::VOTING, + purpose: Purpose::DECRYPTION, security_level: SecurityLevel::HIGH, contract_bounds: None, key_type: KeyType::ECDSA_SECP256K1, @@ -11217,7 +11218,7 @@ mod tests { contact_id: contact, op: PendingContactCryptoOp::RegisterExternal { encrypted_public_key: vec![7u8; 96], - our_decryption_key_index: 0, + our_decryption_key_index: 1, contact_encryption_key_index: 0, }, enqueued_at_ms: 0, @@ -11232,7 +11233,7 @@ mod tests { .await; assert_eq!( drained, 0, - "pass {pass}: a purpose-rejected entry must stay queued, not be cleared" + "pass {pass}: a retryable entry must stay queued, not be cleared" ); } @@ -11246,7 +11247,7 @@ mod tests { assert_eq!( managed.dashpay().pending_contact_crypto.len(), 1, - "the entry must survive repeated drains so a policy change can still pick it up" + "the entry must survive repeated drains so the key, once added, can still pick it up" ); assert!( !managed @@ -11273,7 +11274,7 @@ mod tests { /// ordering fetched first, failed transiently, and left the channel /// intact. /// - /// That ordering is what keeps a purpose-rejected entry — which stays + /// That ordering is what keeps a retryable entry — which stays /// queued by design, and so is retried on every sweep forever — from /// spending a network round trip each time. #[tokio::test] diff --git a/packages/rs-sdk/src/platform/dashpay/contact_request.rs b/packages/rs-sdk/src/platform/dashpay/contact_request.rs index 3f528a4bba6..88cdad40d96 100644 --- a/packages/rs-sdk/src/platform/dashpay/contact_request.rs +++ b/packages/rs-sdk/src/platform/dashpay/contact_request.rs @@ -152,8 +152,7 @@ pub struct SendContactRequestResult { /// request **we are about to mint**. The sender always references its own /// ENCRYPTION key. /// -/// Mint-side only — see [`sender_key_purpose_is_acceptable_on_receive`] for -/// what we accept from documents already on chain. +/// Mint-side only: documents already on chain are not judged by purpose. fn sender_key_purpose_is_valid(purpose: Purpose) -> bool { purpose == Purpose::ENCRYPTION } @@ -171,57 +170,12 @@ fn sender_key_purpose_is_valid(purpose: Purpose) -> bool { /// ECDH is poor key separation, and no new document needs to. /// /// It is NOT the acceptance policy for inbound documents — a `contactRequest` -/// is immutable, so history cannot be re-minted to fit this rule. See -/// [`recipient_key_purpose_is_acceptable_on_receive`]. +/// is immutable, and DIP-15 constrains its keys by type only, so received +/// documents are not judged by purpose at all. pub fn recipient_key_purpose_is_valid(purpose: Purpose) -> bool { matches!(purpose, Purpose::DECRYPTION | Purpose::ENCRYPTION) } -/// Whether `purpose` on the `recipientKeyIndex` key of an **inbound, already -/// on-chain** contact request is acceptable for the ECDH that unwraps the -/// sender's `encryptedPublicKey`. -/// -/// Strictly wider than [`recipient_key_purpose_is_valid`], and deliberately -/// so. `contactRequest` documents are immutable and consensus enforces no -/// purpose constraint on these integer fields, so the acceptance policy is the -/// *only* thing standing between a user and their own payment history. -/// Mainnet device logs (2026-08, a 29-contact wallet whose contacts were -/// established through the legacy Android/dashj client) show 27 of 29 inbound -/// requests referencing the recipient's AUTHENTICATION (key ids 0-2) or -/// TRANSFER (key id 3) key — under the mint-side rule every one of those -/// contacts is unpayable forever, with no action the user can take. -/// -/// Purpose carries no cryptographic weight here: ECDH is defined over the -/// secp256k1 keypair, and DIP-9's identity-key tree is indexed by key *type* -/// and id, never by purpose, so the same derivation reaches all of them. The -/// gates that do carry weight — `ECDSA_SECP256K1` key type and the -/// disabled-key check — are enforced separately by the caller and are -/// unaffected by this predicate. -/// -/// The node-operational purposes (SYSTEM, VOTING, OWNER) stay rejected: -/// nothing on chain references them for DashPay, and they have no business in -/// a payment-channel handshake. -pub fn recipient_key_purpose_is_acceptable_on_receive(purpose: Purpose) -> bool { - matches!( - purpose, - Purpose::DECRYPTION | Purpose::ENCRYPTION | Purpose::AUTHENTICATION | Purpose::TRANSFER - ) -} - -/// Receive-side counterpart of [`sender_key_purpose_is_valid`]: whether -/// `purpose` on the `senderKeyIndex` key of an **inbound, already on-chain** -/// contact request is acceptable for ECDH. -/// -/// Same reasoning as [`recipient_key_purpose_is_acceptable_on_receive`]. The -/// legacy cohort is narrower on this side — the observed mainnet documents -/// pair an AUTHENTICATION sender key with an AUTHENTICATION recipient key — so -/// only AUTHENTICATION is added. A sender referencing any other purpose has -/// not been seen and stays a purpose mismatch (skip-and-retry, never a -/// permanently broken channel), leaving room to widen again on evidence. -pub fn sender_key_purpose_is_acceptable_on_receive(purpose: Purpose) -> bool { - matches!(purpose, Purpose::ENCRYPTION | Purpose::AUTHENTICATION) -} - impl Sdk { /// Create a contact request document /// @@ -708,8 +662,8 @@ mod tests { fn mint_side_still_refuses_authentication_and_transfer() { // What we CREATE stays strict: reusing a signing or fund-authorizing // key for ECDH is poor key separation, and no new document needs to. - // Widening the receive-side acceptance below must never leak into the - // key we pick for our own outgoing requests. + // Accepting every purpose on receive must never leak into the key we + // pick for our own outgoing requests. assert!(!recipient_key_purpose_is_valid(Purpose::AUTHENTICATION)); assert!(!recipient_key_purpose_is_valid(Purpose::TRANSFER)); } @@ -723,51 +677,6 @@ mod tests { assert!(!sender_key_purpose_is_valid(Purpose::AUTHENTICATION)); } - #[test] - fn receive_side_accepts_the_legacy_dashj_cohort() { - // Regression guard for the mainnet legacy cohort: inbound requests - // minted by the Android/dashj client reference the recipient's - // AUTHENTICATION (key ids 0-2) or TRANSFER (key id 3) key. Rejecting - // them made every pre-iOS contact permanently unpayable — the document - // is immutable, so no user action could ever fix it. - for purpose in [ - Purpose::DECRYPTION, - Purpose::ENCRYPTION, - Purpose::AUTHENTICATION, - Purpose::TRANSFER, - ] { - assert!( - recipient_key_purpose_is_acceptable_on_receive(purpose), - "{purpose:?} recipient key must be accepted from an on-chain document" - ); - } - // The sender side of the same legacy documents pairs AUTHENTICATION - // with AUTHENTICATION; ENCRYPTION remains the modern convention. - assert!(sender_key_purpose_is_acceptable_on_receive( - Purpose::ENCRYPTION - )); - assert!(sender_key_purpose_is_acceptable_on_receive( - Purpose::AUTHENTICATION - )); - } - - #[test] - fn receive_side_still_refuses_node_operational_purposes() { - // Not observed on chain for DashPay — widening is evidence-driven, so - // these stay out until something real needs them. A rejection here is - // a skip-and-retry purpose mismatch, never a permanently broken - // channel, so a later widening can still recover those contacts. - for purpose in [Purpose::SYSTEM, Purpose::VOTING, Purpose::OWNER] { - assert!(!recipient_key_purpose_is_acceptable_on_receive(purpose)); - assert!(!sender_key_purpose_is_acceptable_on_receive(purpose)); - } - // TRANSFER is accepted for the recipient (legacy key id 3) but has - // never been seen on the sender side. - assert!(!sender_key_purpose_is_acceptable_on_receive( - Purpose::TRANSFER - )); - } - #[test] fn test_ecdh_shared_secret_symmetry() { // Test that both parties derive the same shared secret diff --git a/packages/rs-sdk/src/platform/dashpay/mod.rs b/packages/rs-sdk/src/platform/dashpay/mod.rs index 3789065cd31..34ac250db7c 100644 --- a/packages/rs-sdk/src/platform/dashpay/mod.rs +++ b/packages/rs-sdk/src/platform/dashpay/mod.rs @@ -10,9 +10,8 @@ mod invitation; mod profile; pub use contact_request::{ - recipient_key_purpose_is_acceptable_on_receive, recipient_key_purpose_is_valid, - sender_key_purpose_is_acceptable_on_receive, ContactRequestInput, ContactRequestResult, - EcdhProvider, RecipientIdentity, SendContactRequestInput, SendContactRequestResult, + recipient_key_purpose_is_valid, ContactRequestInput, ContactRequestResult, EcdhProvider, + RecipientIdentity, SendContactRequestInput, SendContactRequestResult, }; pub use contact_request_queries::ContactRequestDocuments; pub use invitation::{ From 22c6a4cd7ed32102ef58a42bb6fae93db2aa4df1 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:20:40 +0000 Subject: [PATCH 04/15] test: import test helpers at the top and name the merge test with should Co-Authored-By: Claude Opus 5.5 --- .../src/wallet/identity/crypto/validation.rs | 2 +- .../src/wallet/identity/network/update.rs | 7 ++++--- .../rs-sdk/src/platform/transition/update_identity.rs | 10 ++++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs index 60f16aa913d..d6eb12effb9 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -762,7 +762,7 @@ mod tests { /// `merge` must carry the `hard_error` flag so a hard fault in a merged /// sub-result can't be lost. #[test] - fn merge_propagates_hard_error() { + fn should_propagate_a_hard_error_through_merge() { let mut a = ContactRequestValidation::new(); a.add_absent_key_error("absent".into()); let mut b = ContactRequestValidation::new(); diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs index 941b9ad3e57..3d0d02cdb42 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs @@ -291,10 +291,12 @@ impl IdentityWallet { #[cfg(test)] mod tests { use super::*; + use crate::test_support::test_platform_wallet_manager; use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; use dpp::identity::v0::IdentityV0; use std::collections::BTreeMap; + use std::sync::Arc; /// Signs nothing; holds every key so the MASTER key is selectable. #[derive(Debug)] @@ -362,11 +364,10 @@ mod tests { /// would surface as a different error. #[tokio::test] async fn should_refuse_disabling_the_last_critical_key_before_sending_anything() { - let (manager, wallet_id) = crate::test_support::test_platform_wallet_manager().await; + let (manager, wallet_id) = test_platform_wallet_manager().await; let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); { - let persister = - WalletPersister::new(wallet_id, std::sync::Arc::new(NoPlatformPersistence)); + let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); let mut wm = manager.wallet_manager.write().await; let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); info.identity_manager diff --git a/packages/rs-sdk/src/platform/transition/update_identity.rs b/packages/rs-sdk/src/platform/transition/update_identity.rs index f32436b5dfb..29bf8e9b996 100644 --- a/packages/rs-sdk/src/platform/transition/update_identity.rs +++ b/packages/rs-sdk/src/platform/transition/update_identity.rs @@ -182,11 +182,13 @@ impl UpdateIdentity for Identity { #[cfg(test)] mod tests { use super::*; + use dpp::address_funds::AddressWitness; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; use dpp::identity::v0::IdentityV0; use dpp::identity::KeyType; use dpp::platform_value::{BinaryData, Identifier}; + use dpp::ProtocolError; use std::collections::BTreeMap; const NOW_MS: TimestampMillis = 1_800_000_000_000; @@ -364,16 +366,16 @@ mod tests { &self, _key: &IdentityPublicKey, _data: &[u8], - ) -> Result { - Err(dpp::ProtocolError::Generic("not signing".to_string())) + ) -> Result { + Err(ProtocolError::Generic("not signing".to_string())) } async fn sign_create_witness( &self, _key: &IdentityPublicKey, _data: &[u8], - ) -> Result { - Err(dpp::ProtocolError::Generic("not signing".to_string())) + ) -> Result { + Err(ProtocolError::Generic("not signing".to_string())) } fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { From 8037b11fbd1e39347c317ddc21aed9627f795529 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:26:28 +0000 Subject: [PATCH 05/15] refactor: enforce the DIP-11 disable rule as a predicate, leaving update flows as they were Drop the `UpdateIdentity` trait: the revision bump, nonce and broadcast of the wallet's update functions and of wasm-sdk `identityUpdate` return to their previous code, and `update_identity_with_signer` again expects what it expected before. Each path calls `dash_sdk::platform::transition::update_identity::ensure_keys_can_be_disabled` before anything is built or signed, and refuses with `dash_sdk::Error::LastAuthenticationKey`. The rule is unchanged: a stronger non-MASTER key stands in for a weaker level, MASTER needs MASTER, and ECDSA_HASH160 keys count as valid so identities created by older Dash Evo Tool versions keep working. `check_keys_can_be_disabled` stays the pure form, at a given time. Co-Authored-By: Claude Opus 5.5 --- .../src/wallet/identity/network/update.rs | 135 +++++++++---- .../platform/transition/update_identity.rs | 186 ++++++------------ .../src/state_transitions/identity.rs | 58 ++++-- 3 files changed, 196 insertions(+), 183 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs index 3d0d02cdb42..b0510f30e38 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs @@ -18,8 +18,7 @@ use dpp::ProtocolError; use dpp::identity::signer::Signer; use dash_sdk::platform::transition::put_settings::PutSettings; -use dash_sdk::platform::transition::update_identity::UpdateIdentity; -use dpp::state_transition::proof_result::StateTransitionProofResult; +use dash_sdk::platform::transition::update_identity::ensure_keys_can_be_disabled; use crate::error::PlatformWalletError; @@ -74,7 +73,7 @@ impl IdentityWallet { /// MASTER specifically — HIGH/CRITICAL aren't accepted). /// /// An update that would disable the last valid authentication key of a - /// security level is refused before signing with + /// security level is refused before anything is built or signed, with /// `PlatformWalletError::Sdk(dash_sdk::Error::LastAuthenticationKey(_))` /// (DIP-11); keys added in the same update count as valid. /// @@ -105,11 +104,16 @@ impl IdentityWallet { where S: Signer + Send + Sync, { + use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; + use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; + use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; + use dpp::state_transition::proof_result::StateTransitionProofResult; + // Snapshot the local identity + its `identity_index` (needed // for the derivation breadcrumb on the post-broadcast apply // pass below). Read lock only — the broadcast itself doesn't // touch local state. - let (identity, identity_index) = { + let (mut identity, identity_index) = { let wm = self.wallet_manager.read().await; let info = wm.get_wallet_info(&self.wallet_id).ok_or_else(|| { crate::error::PlatformWalletError::WalletNotFound( @@ -132,6 +136,13 @@ impl IdentityWallet { (identity, index) }; + // DIP-11, before the nonce is spent or anything is signed. + ensure_keys_can_be_disabled(&identity, &add_public_keys, &disable_public_keys)?; + + // Increment revision for the update transition. + let original_revision = identity.revision(); + identity.set_revision(original_revision + 1); + // Pick the MASTER signing key — DPP requires identity update // transitions to be authorized by MASTER specifically. let master_key_id = identity @@ -149,27 +160,49 @@ impl IdentityWallet { ) })?; - // The update consumes the keys vecs, so clone before handing - // them off — we need the originals to apply locally after the - // broadcast succeeds. + let identity_nonce = self + .sdk + .get_identity_nonce(identity.id(), true, settings) + .await?; + + let user_fee_increase = settings + .and_then(|s| s.user_fee_increase) + .unwrap_or_default(); + + // `try_from_identity_with_signer` consumes the keys vec, so + // clone before handing it off — we need the originals to + // apply locally after the broadcast succeeds. let added_keys_for_local_apply = add_public_keys.clone(); let disabled_ids_for_local_apply = disable_public_keys.clone(); - // The SDK claims the next revision and refuses, before signing, - // an update that disables the last valid authentication key of - // a security level (DIP-11). Its errors stay typed as - // `PlatformWalletError::Sdk`, so the FFI keeps the consensus code - // and the signer-unavailable classification. - identity - .update_identity_keys( - &self.sdk, - master_key_id, - add_public_keys, - disable_public_keys, - &SignerRef(signer), - settings, - ) - .await?; + let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( + &identity, + &master_key_id, + add_public_keys, + disable_public_keys, + identity_nonce, + user_fee_increase, + &SignerRef(signer), + self.sdk.version(), + None, + ) + .await + .map_err(|e| { + PlatformWalletError::InvalidIdentityData(format!( + "Failed to create identity update transition: {}", + e + )) + })?; + + state_transition + .broadcast_and_wait::(&self.sdk, settings) + .await + .map_err(|e| { + PlatformWalletError::InvalidIdentityData(format!( + "Failed to broadcast identity update: {}", + e + )) + })?; // Post-broadcast local apply. // @@ -259,10 +292,9 @@ impl IdentityWallet { /// caller receives the chain's error. See /// `docs/protocol/authentication-key-limits.md`. /// - /// `identity` is the identity as Platform holds it: the update claims - /// the next revision. An update that would disable the last valid - /// authentication key of a security level is refused before signing - /// with [`dash_sdk::Error::LastAuthenticationKey`] (DIP-11). + /// An update that would disable the last valid authentication key of a + /// security level is refused before anything is built or signed, with + /// [`dash_sdk::Error::LastAuthenticationKey`] (DIP-11). /// /// Returns the [`StateTransitionProofResult`] from the broadcast so callers /// can inspect proof-verified outcomes (e.g. updated keys, balance). @@ -274,17 +306,46 @@ impl IdentityWallet { disable_public_keys: Vec, signer: &S, settings: Option, - ) -> Result { - identity - .update_identity_keys( - &self.sdk, - *master_key_id, - add_public_keys, - disable_public_keys, - signer, - settings, - ) - .await + ) -> Result + { + use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; + use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; + use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; + + // DIP-11, before the nonce is spent or anything is signed. + ensure_keys_can_be_disabled(identity, &add_public_keys, &disable_public_keys)?; + + // Get identity nonce from Platform. + let identity_nonce = self + .sdk + .get_identity_nonce(identity.id(), true, settings) + .await?; + + let user_fee_increase = settings + .and_then(|s| s.user_fee_increase) + .unwrap_or_default(); + + // Build the update transition. + let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( + identity, + master_key_id, + add_public_keys, + disable_public_keys, + identity_nonce, + user_fee_increase, + signer, + self.sdk.version(), + None, + ) + .await + .map_err(dash_sdk::Error::Protocol)?; + + // Broadcast and wait for confirmation. + let result = state_transition + .broadcast_and_wait(&self.sdk, settings) + .await?; + + Ok(result) } } diff --git a/packages/rs-sdk/src/platform/transition/update_identity.rs b/packages/rs-sdk/src/platform/transition/update_identity.rs index 29bf8e9b996..a432499f456 100644 --- a/packages/rs-sdk/src/platform/transition/update_identity.rs +++ b/packages/rs-sdk/src/platform/transition/update_identity.rs @@ -1,31 +1,18 @@ -//! Add and disable an identity's public keys. +//! The DIP-11 rule on disabling an identity's authentication keys. //! -//! [`UpdateIdentity::update_identity_keys`] refuses, before anything is signed, an update that -//! disables the last valid authentication key of a security level. DIP-11: "Keys can only be -//! disabled if another valid key is enabled in the same security level. This can happen -//! atomically in the same state transition." Consensus enforces this for MASTER keys only, so -//! for every other level the client is the only enforcer. [`check_keys_can_be_disabled`] is the -//! same rule as a query, for an interface that wants to offer only the keys it may disable. -//! -//! ```ignore -//! let result = identity -//! .update_identity_keys(&sdk, master_key_id, vec![new_key], vec![old_key_id], &signer, None) -//! .await?; -//! ``` - -use dpp::identity::accessors::{IdentityGettersV0, IdentitySettersV0}; +//! DIP-11: "Keys can only be disabled if another valid key is enabled in the same security +//! level. This can happen atomically in the same state transition." Consensus enforces this for +//! MASTER keys only, so for every other level the client is the only enforcer. Every client path +//! that builds an identity update calls [`ensure_keys_can_be_disabled`] before building or +//! signing anything; [`check_keys_can_be_disabled`] is the same rule at a given time, for an +//! interface that wants to offer only the keys it may disable. + +use dpp::identity::accessors::IdentityGettersV0; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dpp::identity::identity_public_key::accessors::v1::IdentityPublicKeyGettersV1; -use dpp::identity::signer::Signer; use dpp::identity::{Identity, IdentityPublicKey, KeyID, Purpose, SecurityLevel, TimestampMillis}; -use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; -use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; -use dpp::state_transition::proof_result::StateTransitionProofResult; -use crate::platform::transition::broadcast::BroadcastStateTransition; -use crate::platform::transition::put_settings::PutSettings; -use crate::platform::transition::validation::ensure_valid_state_transition_structure; -use crate::{Error, Sdk}; +use crate::Error; /// An identity update would disable the last valid authentication key of a security level. /// @@ -48,9 +35,10 @@ pub struct LastAuthenticationKeyError { /// keeps a valid authentication key at the security level of every authentication key it /// disables, at the time `now_ms` (Unix milliseconds). /// -/// A valid key is enabled and not expired at `now_ms`. The keys the identity keeps count, and -/// so do the keys the same update adds. Ids the identity does not hold are not judged here: -/// Platform refuses them. +/// A valid key is enabled and not expired at `now_ms`, whatever its key type: `ECDSA_HASH160` +/// keys, which identities created by older Dash Evo Tool versions carry, count. The keys the +/// identity keeps count, and so do the keys the same update adds. Ids the identity does not hold +/// are not judged here: Platform refuses them. pub fn check_keys_can_be_disabled( identity: &Identity, add_public_keys: &[IdentityPublicKey], @@ -89,8 +77,20 @@ pub fn check_keys_can_be_disabled( Ok(()) } +/// [`check_keys_can_be_disabled`] at the current wall-clock time, as +/// [`Error::LastAuthenticationKey`]. Call it before building or signing an identity update. +pub fn ensure_keys_can_be_disabled( + identity: &Identity, + add_public_keys: &[IdentityPublicKey], + disable_public_keys: &[KeyID], +) -> Result<(), Error> { + check_keys_can_be_disabled(identity, add_public_keys, disable_public_keys, now_ms()?)?; + Ok(()) +} + /// Whether a key at `candidate` satisfies a requirement for `required`: the same level, or a -/// stronger one other than MASTER, which signs nothing but identity updates. +/// stronger one other than MASTER, which signs nothing but identity updates. The single place +/// the level comparison of this rule is made. fn stands_in_for(candidate: SecurityLevel, required: SecurityLevel) -> bool { candidate == required || (candidate != SecurityLevel::MASTER && candidate.stronger_security_than(required)) @@ -111,84 +111,14 @@ fn now_ms() -> Result { } } -#[async_trait::async_trait] -pub trait UpdateIdentity { - /// Adds `add_public_keys` to this identity and disables the keys `disable_public_keys`, - /// signing with its MASTER key `master_key_id`, and waits for the proved result. - /// - /// `self` is the identity as Platform holds it; the update claims the next revision. - /// Refused before anything is signed, with [`Error::LastAuthenticationKey`], when it would - /// disable the last valid authentication key of a security level: see - /// [`check_keys_can_be_disabled`]. - /// - /// Keys in `add_public_keys` carry their final ids. Their private keys must be reachable - /// through `signer`, which signs each unique-type key's proof of possession as well as the - /// transition. - async fn update_identity_keys>( - &self, - sdk: &Sdk, - master_key_id: KeyID, - add_public_keys: Vec, - disable_public_keys: Vec, - signer: &S, - settings: Option, - ) -> Result; -} - -#[async_trait::async_trait] -impl UpdateIdentity for Identity { - async fn update_identity_keys>( - &self, - sdk: &Sdk, - master_key_id: KeyID, - add_public_keys: Vec, - disable_public_keys: Vec, - signer: &S, - settings: Option, - ) -> Result { - check_keys_can_be_disabled(self, &add_public_keys, &disable_public_keys, now_ms()?)?; - - let mut identity = self.clone(); - let revision = identity - .revision() - .checked_add(1) - .ok_or_else(|| Error::Generic("identity revision overflows".to_string()))?; - identity.set_revision(revision); - - let identity_nonce = sdk - .get_identity_nonce(identity.id(), true, settings) - .await?; - let user_fee_increase = settings - .and_then(|settings| settings.user_fee_increase) - .unwrap_or_default(); - let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( - &identity, - &master_key_id, - add_public_keys, - disable_public_keys, - identity_nonce, - user_fee_increase, - signer, - sdk.version(), - None, - ) - .await?; - ensure_valid_state_transition_structure(&state_transition, sdk.version())?; - - state_transition.broadcast_and_wait(sdk, settings).await - } -} - #[cfg(test)] mod tests { use super::*; - use dpp::address_funds::AddressWitness; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; use dpp::identity::v0::IdentityV0; use dpp::identity::KeyType; use dpp::platform_value::{BinaryData, Identifier}; - use dpp::ProtocolError; use std::collections::BTreeMap; const NOW_MS: TimestampMillis = 1_800_000_000_000; @@ -352,41 +282,34 @@ mod tests { assert!(check_keys_can_be_disabled(&identity_with_critical, &[], &[3], NOW_MS).is_err()); } - /// The refusal comes before the nonce is fetched: the mock answers no request, so a - /// refusal that reached the network would be a different error. - #[cfg(feature = "mocks")] - #[tokio::test] - async fn should_refuse_the_update_before_sending_anything() { - #[derive(Debug)] - struct NoSigner; - - #[async_trait::async_trait] - impl Signer for NoSigner { - async fn sign( - &self, - _key: &IdentityPublicKey, - _data: &[u8], - ) -> Result { - Err(ProtocolError::Generic("not signing".to_string())) - } - - async fn sign_create_witness( - &self, - _key: &IdentityPublicKey, - _data: &[u8], - ) -> Result { - Err(ProtocolError::Generic("not signing".to_string())) - } - - fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { - false - } - } + #[test] + fn should_count_hash160_keys_as_valid() { + // Identities created by older Dash Evo Tool versions carry ECDSA_HASH160 keys. + let hash160 = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: 4, + purpose: Purpose::AUTHENTICATION, + security_level: SecurityLevel::CRITICAL, + contract_bounds: None, + key_type: KeyType::ECDSA_HASH160, + read_only: false, + data: BinaryData::new(vec![7; 20]), + disabled_at: None, + }); + let mut identity = one_key_per_level(); + identity.add_public_key(hash160.clone()); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS), + Ok(()) + ); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[hash160], &[1], NOW_MS), + Ok(()) + ); + } - let sdk = Sdk::new_mock(); - let result = one_key_per_level() - .update_identity_keys(&sdk, 0, vec![], vec![1], &NoSigner, None) - .await; + #[test] + fn should_report_the_refusal_as_a_typed_sdk_error() { + let result = ensure_keys_can_be_disabled(&one_key_per_level(), &[], &[1]); assert!( matches!( result, @@ -397,5 +320,6 @@ mod tests { ), "{result:?}" ); + assert!(ensure_keys_can_be_disabled(&one_key_per_level(), &[], &[2]).is_ok()); } } diff --git a/packages/wasm-sdk/src/state_transitions/identity.rs b/packages/wasm-sdk/src/state_transitions/identity.rs index da20809ad7d..32e1b4852d5 100644 --- a/packages/wasm-sdk/src/state_transitions/identity.rs +++ b/packages/wasm-sdk/src/state_transitions/identity.rs @@ -17,9 +17,10 @@ use dash_sdk::dpp::voting::vote_polls::VotePoll; use dash_sdk::dpp::voting::votes::resource_vote::v0::ResourceVoteV0; use dash_sdk::dpp::voting::votes::resource_vote::ResourceVote; use dash_sdk::dpp::voting::votes::Vote; +use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; use dash_sdk::platform::transition::put_identity::PutIdentity; use dash_sdk::platform::transition::top_up_identity::TopUpIdentity; -use dash_sdk::platform::transition::update_identity::UpdateIdentity; +use dash_sdk::platform::transition::update_identity::ensure_keys_can_be_disabled; use dash_sdk::platform::transition::update_identity_key_limits::{ raised_key_limits, UpdateIdentityKeyLimits, }; @@ -598,7 +599,7 @@ impl WasmSdk { options: IdentityUpdateOptionsJs, ) -> Result<(), WasmSdkError> { // Extract complex types first (borrows &options) - let identity: Identity = IdentityWasm::try_from_options(&options, "identity")?.into(); + let mut identity: Identity = IdentityWasm::try_from_options(&options, "identity")?.into(); let signer = IdentitySignerWasm::try_from_options(&options, "signer")?; let keys_to_add: Vec = if let Some(keys_array) = try_from_options_optional_with(&options, "addPublicKeys", |v| { @@ -636,6 +637,12 @@ impl WasmSdk { // Deserialize simple fields last (consumes options) let parsed = deserialize_identity_update_options(options.into())?; + // Increment the identity revision for the update transition + // The platform expects the new revision (current + 1) in the state transition + use dash_sdk::dpp::identity::accessors::IdentitySettersV0; + let original_revision = identity.revision(); + identity.set_revision(original_revision + 1); + // Find all valid master keys (AUTHENTICATION + MASTER + supported key type) let master_keys: Vec<_> = identity .public_keys() @@ -669,20 +676,41 @@ impl WasmSdk { // Get keys to disable let keys_to_disable = parsed.disable_public_keys.unwrap_or_default(); - // The SDK claims the next revision and refuses, before signing, an - // update that disables the last valid authentication key of a - // security level (DIP-11). - identity - .update_identity_keys( - self.inner_sdk(), - master_key_id, - keys_to_add, - keys_to_disable, - &signer, - settings, - ) + // DIP-11, before the nonce is spent or anything is signed. + ensure_keys_can_be_disabled(&identity, &keys_to_add, &keys_to_disable)?; + + // Get identity nonce + let identity_nonce = self + .inner_sdk() + .get_identity_nonce(identity.id(), true, settings) + .await + .map_err(|e| WasmSdkError::generic(format!("Failed to get identity nonce: {}", e)))?; + + // Create the identity update transition + use crate::settings::get_user_fee_increase; + use dash_sdk::dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; + use dash_sdk::dpp::state_transition::identity_update_transition::IdentityUpdateTransition; + + let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( + &identity, + &master_key_id, + keys_to_add, + keys_to_disable, + identity_nonce, + get_user_fee_increase(settings.as_ref()), + &signer, + self.inner_sdk().version(), + None, + ) + .await + .map_err(|e| WasmSdkError::with_context("Failed to create update transition", e))?; + + // Broadcast the transition + use dash_sdk::dpp::state_transition::proof_result::StateTransitionProofResult; + state_transition + .broadcast_and_wait::(self.inner_sdk(), settings) .await - .map_err(|e| WasmSdkError::with_context("Failed to update identity", e))?; + .map_err(|e| WasmSdkError::with_context("Failed to broadcast update", e))?; Ok(()) } From 3c313e86d49c99c96225b5fbb4a321dfc05b4b6a Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:28:57 +0000 Subject: [PATCH 06/15] fix(platform-wallet): log key warnings on accepted contact requests and test both update entry points A received contact request naming a since-disabled key is accepted with a warning; the drain and the accept path dropped that warning. Log it at warn level so the downgrade stays observable. Cover `update_identity_with_signer` as well as `update_identity_with_external_signer`: both refuse disabling the last valid authentication key of a level before the nonce is fetched. Co-Authored-By: Claude Opus 5.5 --- .../identity/network/contact_requests.rs | 9 ++++++++ .../src/wallet/identity/network/update.rs | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index c4c27b31a0b..dc2fbacb852 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -2459,6 +2459,9 @@ impl DashPayView<'_, B> { } continue; } + for warning in &recipient_validation.warnings { + tracing::warn!(owner = %entry.owner_identity_id, contact = %entry.contact_id, warning, "drain: contact request accepted with a key warning"); + } // Fetch the contact identity (transient on failure → leave). // Bounded: a Platform round trip, and nothing in this entry @@ -2519,6 +2522,9 @@ impl DashPayView<'_, B> { } continue; } + for warning in &validation.warnings { + tracing::warn!(owner = %entry.owner_identity_id, contact = %entry.contact_id, warning, "drain: contact request accepted with a key warning"); + } // The contact's encryption pubkey (peer). A malformed/missing // key is a permanent fault — re-deriving won't help. @@ -3724,6 +3730,9 @@ impl DashPayView<'_, B> { validation.errors ))); } + for warning in &validation.warnings { + tracing::warn!(contact = %contact_id, warning, "accept: contact request accepted with a key warning"); + } // Seedless ECDH: our decryption-key scalar (at the Rust-built path) // against the contact's encryption pubkey, computed in the signer. The diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs index b0510f30e38..b892a29a985 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs @@ -455,4 +455,25 @@ mod tests { "{result:?}" ); } + + /// The caller-supplied-identity entry point refuses the same way, before + /// the nonce is fetched. + #[tokio::test] + async fn should_refuse_disabling_the_last_critical_key_with_a_supplied_identity() { + let (manager, wallet_id) = test_platform_wallet_manager().await; + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + + let result = wallet + .identity() + .update_identity_with_signer(&identity(), &0, vec![], vec![1], &HoldsEveryKey, None) + .await; + assert!( + matches!( + result, + Err(dash_sdk::Error::LastAuthenticationKey(ref refusal)) + if refusal.key_id == 1 && refusal.security_level == SecurityLevel::CRITICAL + ), + "{result:?}" + ); + } } From f0b82ef4d7abff79d0cf76a47f9a9a62f44b160a Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:39:32 +0000 Subject: [PATCH 07/15] fix(platform-wallet): name the owner identity in the accept-path key warning Co-Authored-By: Claude Opus 5.5 --- .../src/wallet/identity/network/contact_requests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index dc2fbacb852..7eabc52309e 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -3731,7 +3731,7 @@ impl DashPayView<'_, B> { ))); } for warning in &validation.warnings { - tracing::warn!(contact = %contact_id, warning, "accept: contact request accepted with a key warning"); + tracing::warn!(owner = %our_identity_id, contact = %contact_id, warning, "accept: contact request accepted with a key warning"); } // Seedless ECDH: our decryption-key scalar (at the Rust-built path) From cfbbd4ccd6edaafc5502aa4936861aba82b3e1b1 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:15:17 +0000 Subject: [PATCH 08/15] refactor(platform-wallet): name contact-request key sides with an enum The sender and recipient labels in the contact-request key validation messages were string literals repeated across the send and receive validators. A ContactRequestKeySide enum with Display now supplies them; the message text is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../src/wallet/identity/crypto/validation.rs | 76 +++++++++++++++---- 1 file changed, 61 insertions(+), 15 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs index d6eb12effb9..a661d45f951 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -8,6 +8,26 @@ use dpp::identity::accessors::IdentityGettersV0; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dpp::identity::{Identity, KeyType}; +use std::fmt; + +/// The side of a contact request a key index refers to, as named in +/// validation messages. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ContactRequestKeySide { + /// The identity that sent the request (`senderKeyIndex`). + Sender, + /// The identity the request is addressed to (`recipientKeyIndex`). + Recipient, +} + +impl fmt::Display for ContactRequestKeySide { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(match self { + Self::Sender => "Sender", + Self::Recipient => "Recipient", + }) + } +} /// Result of validating a contact request before it is sent. #[derive(Debug, Clone)] @@ -172,8 +192,16 @@ pub(crate) fn validate_contact_request_to_send( recipient_key_index, ); for (side, identity, key_index) in [ - ("Sender", sender_identity, sender_key_index), - ("Recipient", recipient_identity, recipient_key_index), + ( + ContactRequestKeySide::Sender, + sender_identity, + sender_key_index, + ), + ( + ContactRequestKeySide::Recipient, + recipient_identity, + recipient_key_index, + ), ] { if let Some(disabled_at) = identity .get_public_key_by_id(key_index) @@ -206,7 +234,8 @@ pub(crate) fn validate_sender_key( // Must be ECDSA_SECP256K1 for ECDH. if key.key_type() != KeyType::ECDSA_SECP256K1 { validation.add_error(format!( - "Sender key {} has type {:?}, but ECDSA_SECP256K1 is required for ECDH", + "{} key {} has type {:?}, but ECDSA_SECP256K1 is required for ECDH", + ContactRequestKeySide::Sender, sender_key_index, key.key_type(), )); @@ -215,15 +244,18 @@ pub(crate) fn validate_sender_key( // Disabled since the request was sent: still usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { validation.add_warning(format!( - "Sender key {} is disabled (at timestamp {}); accepted, as the request \ + "{} key {} is disabled (at timestamp {}); accepted, as the request \ named it before", - sender_key_index, disabled_at, + ContactRequestKeySide::Sender, + sender_key_index, + disabled_at, )); } } None => { validation.add_absent_key_error(format!( - "Sender key index {} not found on identity {}", + "{} key index {} not found on identity {}", + ContactRequestKeySide::Sender, sender_key_index, sender_identity.id(), )); @@ -277,15 +309,18 @@ pub(crate) fn validate_recipient_key( } KeyType::ECDSA_HASH160 => { validation.add_warning(format!( - "Recipient key {} is ECDSA_HASH160; full public key is needed for ECDH — \ + "{} key {} is ECDSA_HASH160; full public key is needed for ECDH — \ ensure the full key is available", + ContactRequestKeySide::Recipient, recipient_key_index, )); } other => { validation.add_error(format!( - "Recipient key {} has type {:?}, which is not compatible with ECDH", - recipient_key_index, other, + "{} key {} has type {:?}, which is not compatible with ECDH", + ContactRequestKeySide::Recipient, + recipient_key_index, + other, )); } } @@ -294,15 +329,18 @@ pub(crate) fn validate_recipient_key( // scalar, so the request stays usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { validation.add_warning(format!( - "Recipient key {} is disabled (at timestamp {}); accepted, as the request \ + "{} key {} is disabled (at timestamp {}); accepted, as the request \ named it before", - recipient_key_index, disabled_at, + ContactRequestKeySide::Recipient, + recipient_key_index, + disabled_at, )); } } None => { validation.add_absent_key_error(format!( - "Recipient key index {} not found on identity {}", + "{} key index {} not found on identity {}", + ContactRequestKeySide::Recipient, recipient_key_index, recipient_identity.id(), )); @@ -539,8 +577,16 @@ mod tests { assert!(valid.is_valid, "errors: {:?}", valid.errors); for (sender, recipient, side) in [ - (&disabled_sender, &enabled_recipient, "Sender"), - (&enabled_sender, &disabled_recipient, "Recipient"), + ( + &disabled_sender, + &enabled_recipient, + ContactRequestKeySide::Sender, + ), + ( + &enabled_sender, + &disabled_recipient, + ContactRequestKeySide::Recipient, + ), ] { let result = validate_contact_request_to_send(sender, 0, recipient, 0); assert!( @@ -551,7 +597,7 @@ mod tests { result .errors .iter() - .any(|e| e.starts_with(side) && e.contains("disabled")), + .any(|e| e.starts_with(&side.to_string()) && e.contains("disabled")), "errors: {:?}", result.errors ); From 0e66b753bce462573ca8da8b30b2ea8f6ea8de8a Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 28 Oct 2026 16:04:41 +0000 Subject: [PATCH 09/15] refactor: enforce the DIP-11 last-key rule in dpp's identity update builder The rule "keys can only be disabled if another valid key is enabled in the same security level" lived in rs-sdk, and every client path had to remember to call it. It is a pure function over identity data, so it moves to dpp (`identity::key_disable_rule::check_keys_can_be_disabled`) and `IdentityUpdateTransitionV0::try_from_identity_with_signer` applies it before signing anything. The wallet, wasm-sdk and any other client of the builder now get it without caller code. The refusal is `ProtocolError::LastAuthenticationKey`: signing-side policy, not consensus (Drive guards MASTER only). dpp reads no clock, so the builder does not count a key with any `expires_at` as remaining; callers with a clock pass `Some(now_ms)` to the predicate. Removed: rs-sdk `platform::transition::update_identity` and `dash_sdk::Error::LastAuthenticationKey`, with the wallet and wasm-sdk calls. The wallet keeps the refusal typed on both entry points, and wasm-sdk reports it as an invalid argument. strategy-tests build their DisableKeys transition by hand: they exercise consensus, which accepts disabling a non-MASTER level's last key. A test pins that the hand-built transition equals the builder's whenever the builder accepts it. Co-Authored-By: Claude Opus 5.5 --- packages/rs-dpp/src/errors/protocol_error.rs | 7 + .../src/identity/key_disable_rule.rs} | 233 ++++++++++-------- packages/rs-dpp/src/identity/mod.rs | 1 + .../methods/v0/mod.rs | 8 + .../v0/v0_methods.rs | 222 +++++++++++++++++ .../rs-platform-wallet/src/test_support.rs | 25 +- .../src/wallet/identity/network/update.rs | 66 +++-- packages/rs-sdk/src/error.rs | 6 - packages/rs-sdk/src/platform/transition.rs | 1 - packages/strategy-tests/src/transitions.rs | 133 ++++++++-- packages/wasm-sdk/src/error.rs | 35 ++- .../src/state_transitions/identity.rs | 12 +- 12 files changed, 586 insertions(+), 163 deletions(-) rename packages/{rs-sdk/src/platform/transition/update_identity.rs => rs-dpp/src/identity/key_disable_rule.rs} (59%) diff --git a/packages/rs-dpp/src/errors/protocol_error.rs b/packages/rs-dpp/src/errors/protocol_error.rs index 1cc5106db22..90cb7b50392 100644 --- a/packages/rs-dpp/src/errors/protocol_error.rs +++ b/packages/rs-dpp/src/errors/protocol_error.rs @@ -15,6 +15,7 @@ use crate::document::errors::*; ))] use crate::state_transition::errors::InvalidIdentityPublicKeyTypeError; +use crate::identity::key_disable_rule::LastAuthenticationKeyError; #[cfg(all(feature = "state-transitions", feature = "validation"))] use crate::state_transition::errors::StateTransitionError; @@ -324,6 +325,12 @@ pub enum ProtocolError { using: u16, msg: &'static str, }, + + /// An identity update would disable the last valid authentication key of a security + /// level (DIP-11). Refused by the signing builder before anything is signed; this is + /// client-side policy, not a consensus rule, so it is never a `ConsensusError`. + #[error(transparent)] + LastAuthenticationKey(#[from] LastAuthenticationKeyError), } impl From<&str> for ProtocolError { diff --git a/packages/rs-sdk/src/platform/transition/update_identity.rs b/packages/rs-dpp/src/identity/key_disable_rule.rs similarity index 59% rename from packages/rs-sdk/src/platform/transition/update_identity.rs rename to packages/rs-dpp/src/identity/key_disable_rule.rs index a432499f456..2332ad773ab 100644 --- a/packages/rs-sdk/src/platform/transition/update_identity.rs +++ b/packages/rs-dpp/src/identity/key_disable_rule.rs @@ -1,20 +1,25 @@ //! The DIP-11 rule on disabling an identity's authentication keys. //! //! DIP-11: "Keys can only be disabled if another valid key is enabled in the same security -//! level. This can happen atomically in the same state transition." Consensus enforces this for -//! MASTER keys only, so for every other level the client is the only enforcer. Every client path -//! that builds an identity update calls [`ensure_keys_can_be_disabled`] before building or -//! signing anything; [`check_keys_can_be_disabled`] is the same rule at a given time, for an -//! interface that wants to offer only the keys it may disable. - -use dpp::identity::accessors::IdentityGettersV0; -use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; -use dpp::identity::identity_public_key::accessors::v1::IdentityPublicKeyGettersV1; -use dpp::identity::{Identity, IdentityPublicKey, KeyID, Purpose, SecurityLevel, TimestampMillis}; +//! level. This can happen atomically in the same state transition." +//! +//! **This is signing-side policy, not consensus.** Drive enforces it for MASTER keys only and +//! accepts an identity update that disables the last key of any other level, so this module +//! never produces a `ConsensusError` and nothing on a block-execution path calls it. Clients +//! enforce it: `IdentityUpdateTransition::try_from_identity_with_signer` (the +//! `state-transition-signing` builder) refuses such an update before signing anything, and an +//! interface with a clock can call [`check_keys_can_be_disabled`] to offer only the keys it may +//! disable. -use crate::Error; +use crate::identity::accessors::IdentityGettersV0; +use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; +use crate::identity::identity_public_key::accessors::v1::IdentityPublicKeyGettersV1; +use crate::identity::{Identity, IdentityPublicKey, KeyID, Purpose, SecurityLevel}; +use crate::prelude::TimestampMillis; -/// An identity update would disable the last valid authentication key of a security level. +/// An identity update would disable the last valid authentication key of a security level, +/// which DIP-11 forbids. Signing-side policy, not a consensus error: Platform would accept the +/// update for every level but MASTER. /// /// A security level is a minimum: a key of a stronger level stands in for it, except MASTER, /// which signs identity updates only. So a CRITICAL key keeps a HIGH key disableable, a MASTER @@ -33,17 +38,24 @@ pub struct LastAuthenticationKeyError { /// Whether an identity update that adds `add_public_keys` and disables `disable_public_keys` /// keeps a valid authentication key at the security level of every authentication key it -/// disables, at the time `now_ms` (Unix milliseconds). +/// disables. +/// +/// A valid key is an enabled authentication key that has not expired, whatever its key type: +/// `ECDSA_HASH160` keys, which identities created by older Dash Evo Tool versions carry, count. +/// The keys the identity keeps count, and so do the keys the same update adds. /// -/// A valid key is enabled and not expired at `now_ms`, whatever its key type: `ECDSA_HASH160` -/// keys, which identities created by older Dash Evo Tool versions carry, count. The keys the -/// identity keeps count, and so do the keys the same update adds. Ids the identity does not hold -/// are not judged here: Platform refuses them. +/// `now_ms` is the caller's current time in Unix milliseconds. dpp reads no clock, so `None` +/// judges conservatively: a key with any `expires_at` does not count as remaining, which is +/// stricter than DIP-11 and therefore still compliant. With `Some(now)`, a key counts until +/// `now` reaches its `expires_at`. +/// +/// Ids the identity does not hold, and keys of other purposes, are not judged here: Platform +/// refuses unknown ids, and DIP-11 speaks of authentication keys. pub fn check_keys_can_be_disabled( identity: &Identity, add_public_keys: &[IdentityPublicKey], disable_public_keys: &[KeyID], - now_ms: TimestampMillis, + now_ms: Option, ) -> Result<(), LastAuthenticationKeyError> { let remaining_valid_keys = || { identity @@ -54,7 +66,7 @@ pub fn check_keys_can_be_disabled( .filter(|key| { key.purpose() == Purpose::AUTHENTICATION && key.disabled_at().is_none() - && !key.is_expired_at(now_ms) + && is_unexpired(key, now_ms) }) }; for key_id in disable_public_keys { @@ -77,15 +89,12 @@ pub fn check_keys_can_be_disabled( Ok(()) } -/// [`check_keys_can_be_disabled`] at the current wall-clock time, as -/// [`Error::LastAuthenticationKey`]. Call it before building or signing an identity update. -pub fn ensure_keys_can_be_disabled( - identity: &Identity, - add_public_keys: &[IdentityPublicKey], - disable_public_keys: &[KeyID], -) -> Result<(), Error> { - check_keys_can_be_disabled(identity, add_public_keys, disable_public_keys, now_ms()?)?; - Ok(()) +/// Whether `key` is unexpired at `now_ms`; without a time, only a key with no expiry is. +fn is_unexpired(key: &IdentityPublicKey, now_ms: Option) -> bool { + match now_ms { + Some(now_ms) => !key.is_expired_at(now_ms), + None => key.expires_at().is_none(), + } } /// Whether a key at `candidate` satisfies a requirement for `required`: the same level, or a @@ -96,51 +105,53 @@ fn stands_in_for(candidate: SecurityLevel, required: SecurityLevel) -> bool { || (candidate != SecurityLevel::MASTER && candidate.stronger_security_than(required)) } -/// The current wall-clock time in Unix milliseconds, on native targets and in the browser. -fn now_ms() -> Result { - #[cfg(not(target_arch = "wasm32"))] - { - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|elapsed| elapsed.as_millis() as TimestampMillis) - .map_err(|e| Error::Generic(format!("system time is before the Unix epoch: {e}"))) - } - #[cfg(target_arch = "wasm32")] - { - Ok(js_sys::Date::now() as TimestampMillis) - } -} - #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; - use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; - use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; - use dpp::identity::v0::IdentityV0; - use dpp::identity::KeyType; - use dpp::platform_value::{BinaryData, Identifier}; + use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; + use crate::identity::identity_public_key::v0::IdentityPublicKeyV0; + use crate::identity::v0::IdentityV0; + use crate::identity::KeyType; + use platform_value::{BinaryData, Identifier}; use std::collections::BTreeMap; const NOW_MS: TimestampMillis = 1_800_000_000_000; - fn key(id: KeyID, purpose: Purpose, security_level: SecurityLevel) -> IdentityPublicKey { + pub(crate) fn key( + id: KeyID, + purpose: Purpose, + security_level: SecurityLevel, + key_type: KeyType, + ) -> IdentityPublicKey { + let data = match key_type { + KeyType::ECDSA_HASH160 => vec![id as u8; 20], + _ => vec![id as u8; 33], + }; IdentityPublicKey::V0(IdentityPublicKeyV0 { id, purpose, security_level, contract_bounds: None, - key_type: KeyType::ECDSA_SECP256K1, + key_type, read_only: false, - data: BinaryData::new(vec![id as u8; 33]), + data: BinaryData::new(data), disabled_at: None, }) } - fn authentication_key(id: KeyID, security_level: SecurityLevel) -> IdentityPublicKey { - key(id, Purpose::AUTHENTICATION, security_level) + pub(crate) fn authentication_key( + id: KeyID, + security_level: SecurityLevel, + ) -> IdentityPublicKey { + key( + id, + Purpose::AUTHENTICATION, + security_level, + KeyType::ECDSA_SECP256K1, + ) } - fn identity(keys: Vec) -> Identity { + pub(crate) fn identity(keys: Vec) -> Identity { IdentityV0 { id: Identifier::from([1; 32]), public_keys: keys @@ -154,7 +165,7 @@ mod tests { } /// MASTER 0, CRITICAL 1, HIGH 2, MEDIUM 3: one authentication key per level. - fn one_key_per_level() -> Identity { + pub(crate) fn one_key_per_level() -> Identity { identity(vec![ authentication_key(0, SecurityLevel::MASTER), authentication_key(1, SecurityLevel::CRITICAL), @@ -165,7 +176,7 @@ mod tests { #[test] fn should_refuse_disabling_the_only_critical_key() { - let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[1], NOW_MS); + let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[1], Some(NOW_MS)); assert_eq!( result, Err(LastAuthenticationKeyError { @@ -177,7 +188,7 @@ mod tests { #[test] fn should_refuse_disabling_the_only_master_key_without_adding_one() { - let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[0], NOW_MS); + let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[0], Some(NOW_MS)); assert_eq!( result, Err(LastAuthenticationKeyError { @@ -191,7 +202,7 @@ mod tests { fn should_let_a_stronger_non_master_key_stand_in_for_a_weaker_level() { // CRITICAL 1 still meets HIGH and MEDIUM requirements. assert_eq!( - check_keys_can_be_disabled(&one_key_per_level(), &[], &[2, 3], NOW_MS), + check_keys_can_be_disabled(&one_key_per_level(), &[], &[2, 3], Some(NOW_MS)), Ok(()) ); } @@ -203,19 +214,24 @@ mod tests { authentication_key(0, SecurityLevel::MASTER), authentication_key(1, SecurityLevel::CRITICAL), ]); - assert!(check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS).is_err()); + assert!(check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)).is_err()); } #[test] fn should_allow_disabling_a_key_replaced_in_the_same_update() { let replacement = authentication_key(4, SecurityLevel::CRITICAL); assert_eq!( - check_keys_can_be_disabled(&one_key_per_level(), &[replacement], &[1], NOW_MS), + check_keys_can_be_disabled(&one_key_per_level(), &[replacement], &[1], Some(NOW_MS)), Ok(()) ); let master_replacement = authentication_key(4, SecurityLevel::MASTER); assert_eq!( - check_keys_can_be_disabled(&one_key_per_level(), &[master_replacement], &[0], NOW_MS), + check_keys_can_be_disabled( + &one_key_per_level(), + &[master_replacement], + &[0], + Some(NOW_MS) + ), Ok(()) ); } @@ -228,10 +244,10 @@ mod tests { authentication_key(2, SecurityLevel::CRITICAL), ]); assert_eq!( - check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS), + check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)), Ok(()) ); - assert!(check_keys_can_be_disabled(&identity, &[], &[1, 2], NOW_MS).is_err()); + assert!(check_keys_can_be_disabled(&identity, &[], &[1, 2], Some(NOW_MS)).is_err()); } #[test] @@ -246,18 +262,45 @@ mod tests { disabled, expired, ]); - assert!(check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS).is_err()); + assert!(check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)).is_err()); let expired_replacement = authentication_key(4, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS)); assert!( - check_keys_can_be_disabled(&identity, &[expired_replacement], &[1], NOW_MS).is_err() + check_keys_can_be_disabled(&identity, &[expired_replacement], &[1], Some(NOW_MS)) + .is_err() ); let unexpired = authentication_key(5, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS + 1)); assert_eq!( - check_keys_can_be_disabled(&identity, &[unexpired], &[1], NOW_MS), + check_keys_can_be_disabled(&identity, &[unexpired], &[1], Some(NOW_MS)), + Ok(()) + ); + } + + #[test] + fn should_not_count_a_key_with_any_expiry_without_a_clock() { + // Without a time the rule cannot tell an expiring key from an expired one, so it counts + // neither: stricter than DIP-11, never looser. + let far_future = + authentication_key(4, SecurityLevel::CRITICAL).with_limits(None, Some(u64::MAX)); + let mut identity = one_key_per_level(); + identity.add_public_key(far_future.clone()); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], None), + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + assert!( + check_keys_can_be_disabled(&one_key_per_level(), &[far_future], &[1], None).is_err() + ); + // A key without limits counts the same either way. + let replacement = authentication_key(5, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[replacement], &[1], None), Ok(()) ); } @@ -266,11 +309,21 @@ mod tests { fn should_not_judge_keys_of_other_purposes_or_unknown_ids() { let identity = identity(vec![ authentication_key(0, SecurityLevel::MASTER), - key(1, Purpose::TRANSFER, SecurityLevel::CRITICAL), - key(2, Purpose::ENCRYPTION, SecurityLevel::MEDIUM), + key( + 1, + Purpose::TRANSFER, + SecurityLevel::CRITICAL, + KeyType::ECDSA_SECP256K1, + ), + key( + 2, + Purpose::ENCRYPTION, + SecurityLevel::MEDIUM, + KeyType::ECDSA_SECP256K1, + ), ]); assert_eq!( - check_keys_can_be_disabled(&identity, &[], &[1, 2, 9], NOW_MS), + check_keys_can_be_disabled(&identity, &[], &[1, 2, 9], Some(NOW_MS)), Ok(()) ); // A non-authentication key does not keep an authentication level valid. @@ -279,47 +332,29 @@ mod tests { identity.add_public_key(authentication_key(3, SecurityLevel::CRITICAL)); identity }; - assert!(check_keys_can_be_disabled(&identity_with_critical, &[], &[3], NOW_MS).is_err()); + assert!( + check_keys_can_be_disabled(&identity_with_critical, &[], &[3], Some(NOW_MS)).is_err() + ); } #[test] fn should_count_hash160_keys_as_valid() { // Identities created by older Dash Evo Tool versions carry ECDSA_HASH160 keys. - let hash160 = IdentityPublicKey::V0(IdentityPublicKeyV0 { - id: 4, - purpose: Purpose::AUTHENTICATION, - security_level: SecurityLevel::CRITICAL, - contract_bounds: None, - key_type: KeyType::ECDSA_HASH160, - read_only: false, - data: BinaryData::new(vec![7; 20]), - disabled_at: None, - }); + let hash160 = key( + 4, + Purpose::AUTHENTICATION, + SecurityLevel::CRITICAL, + KeyType::ECDSA_HASH160, + ); let mut identity = one_key_per_level(); identity.add_public_key(hash160.clone()); assert_eq!( - check_keys_can_be_disabled(&identity, &[], &[1], NOW_MS), + check_keys_can_be_disabled(&identity, &[], &[1], None), Ok(()) ); assert_eq!( - check_keys_can_be_disabled(&one_key_per_level(), &[hash160], &[1], NOW_MS), + check_keys_can_be_disabled(&one_key_per_level(), &[hash160], &[1], None), Ok(()) ); } - - #[test] - fn should_report_the_refusal_as_a_typed_sdk_error() { - let result = ensure_keys_can_be_disabled(&one_key_per_level(), &[], &[1]); - assert!( - matches!( - result, - Err(Error::LastAuthenticationKey(LastAuthenticationKeyError { - key_id: 1, - security_level: SecurityLevel::CRITICAL, - })) - ), - "{result:?}" - ); - assert!(ensure_keys_can_be_disabled(&one_key_per_level(), &[], &[2]).is_ok()); - } } diff --git a/packages/rs-dpp/src/identity/mod.rs b/packages/rs-dpp/src/identity/mod.rs index 14ddeb8fd2c..77fc41a00ae 100644 --- a/packages/rs-dpp/src/identity/mod.rs +++ b/packages/rs-dpp/src/identity/mod.rs @@ -26,6 +26,7 @@ mod identity_facade; #[cfg(feature = "factories")] pub mod identity_factory; pub mod identity_nonce; +pub mod key_disable_rule; pub mod methods; #[cfg(feature = "random-identities")] pub mod random; diff --git a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs index 9c0d00c3f80..e06373d6c75 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs @@ -15,6 +15,14 @@ use crate::{identity::KeyID, ProtocolError}; use platform_version::version::PlatformVersion; pub trait IdentityUpdateTransitionMethodsV0 { + /// Builds and signs an identity update that adds `add_public_keys` and disables + /// `disable_public_keys`, signed by the MASTER key `master_public_key_id`. + /// + /// Refuses, before signing anything, an update that would disable the last valid + /// authentication key of a security level, with [`ProtocolError::LastAuthenticationKey`] + /// (DIP-11 signing-side policy; see + /// [`check_keys_can_be_disabled`](crate::identity::key_disable_rule::check_keys_can_be_disabled)). + /// The builder has no clock, so a key with any expiry does not count as remaining. #[cfg(feature = "state-transition-signing")] #[allow(clippy::too_many_arguments)] async fn try_from_identity_with_signer>( diff --git a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs index a975040c95f..1296c71242f 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs @@ -19,6 +19,8 @@ use crate::identity::{Identity, IdentityPublicKey}; use crate::identity::accessors::IdentityGettersV0; #[cfg(feature = "state-transition-signing")] use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; +#[cfg(feature = "state-transition-signing")] +use crate::identity::key_disable_rule::check_keys_can_be_disabled; use crate::prelude::IdentityNonce; #[cfg(feature = "state-transition-signing")] use crate::prelude::UserFeeIncrease; @@ -52,6 +54,13 @@ impl IdentityUpdateTransitionMethodsV0 for IdentityUpdateTransitionV0 { _platform_version: &PlatformVersion, _version: Option, ) -> Result { + // DIP-11 signing-side policy, checked before anything is signed. This is an in-place edit + // of a generation every protocol version selects, and it is inert for consensus at all of + // them: only clients build transitions here (no Drive or drive-abci code path calls this + // builder), and a request the rule accepts yields the same transition as before. dpp + // reads no clock, so a key with any expiry does not count as remaining (`None`). + check_keys_can_be_disabled(identity, &add_public_keys, &disable_public_keys, None)?; + let add_public_keys_in_creation = add_public_keys .iter() .map(|public_key| public_key.into()) @@ -159,3 +168,216 @@ impl IdentityUpdateTransitionAccessorsV0 for IdentityUpdateTransitionV0 { &self.disable_public_keys } } + +#[cfg(all(test, feature = "state-transition-signing"))] +mod tests { + use crate::address_funds::AddressWitness; + use crate::identity::accessors::IdentityGettersV0; + use crate::identity::key_disable_rule::tests::{authentication_key, key, one_key_per_level}; + use crate::identity::key_disable_rule::LastAuthenticationKeyError; + use crate::identity::signer::Signer; + use crate::identity::{Identity, IdentityPublicKey, KeyID, KeyType, Purpose, SecurityLevel}; + use crate::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; + use crate::state_transition::identity_update_transition::v0::IdentityUpdateTransitionV0; + use crate::state_transition::identity_update_transition::IdentityUpdateTransition; + use crate::state_transition::public_key_in_creation::accessors::IdentityPublicKeyInCreationV0Setters; + use crate::state_transition::public_key_in_creation::IdentityPublicKeyInCreation; + use crate::state_transition::StateTransition; + use crate::ProtocolError; + use assert_matches::assert_matches; + use async_trait::async_trait; + use platform_value::BinaryData; + use platform_version::version::{PlatformVersion, PLATFORM_VERSIONS}; + use std::sync::atomic::{AtomicUsize, Ordering}; + + const SIGNATURE: [u8; 65] = [7; 65]; + + /// Returns [`SIGNATURE`] for every request and counts them. + #[derive(Debug, Default)] + struct CountingSigner { + signatures: AtomicUsize, + } + + impl CountingSigner { + fn signatures(&self) -> usize { + self.signatures.load(Ordering::SeqCst) + } + } + + #[async_trait] + impl Signer for CountingSigner { + async fn sign( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + self.signatures.fetch_add(1, Ordering::SeqCst); + Ok(SIGNATURE.to_vec().into()) + } + + async fn sign_create_witness( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(ProtocolError::Generic( + "identity updates create no address witnesses".to_string(), + )) + } + + fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { + true + } + } + + async fn build( + identity: &Identity, + add_public_keys: Vec, + disable_public_keys: Vec, + signer: &CountingSigner, + platform_version: &PlatformVersion, + ) -> Result { + IdentityUpdateTransition::try_from_identity_with_signer( + identity, + &0, + add_public_keys, + disable_public_keys, + 5, + 0, + signer, + platform_version, + None, + ) + .await + } + + #[tokio::test] + async fn should_refuse_disabling_the_last_critical_key_before_signing_anything() { + let signer = CountingSigner::default(); + // The added key is a unique key type, so a build that got past the rule would sign it. + let added = authentication_key(4, SecurityLevel::MEDIUM); + let result = build( + &one_key_per_level(), + vec![added], + vec![1], + &signer, + PlatformVersion::latest(), + ) + .await; + assert_matches!( + result, + Err(ProtocolError::LastAuthenticationKey( + LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + } + )) + ); + assert_eq!(signer.signatures(), 0, "nothing may be signed"); + } + + #[tokio::test] + async fn should_not_count_an_expiring_key_as_remaining_in_the_builder() { + // The builder has no clock: even an expiry far in the future does not count. + let mut identity = one_key_per_level(); + identity.add_public_key( + authentication_key(4, SecurityLevel::CRITICAL).with_limits(None, Some(u64::MAX)), + ); + let signer = CountingSigner::default(); + let result = build( + &identity, + vec![], + vec![1], + &signer, + PlatformVersion::latest(), + ) + .await; + assert_matches!( + result, + Err(ProtocolError::LastAuthenticationKey( + LastAuthenticationKeyError { key_id: 1, .. } + )) + ); + assert_eq!(signer.signatures(), 0); + + // The same key without the expiry keeps key 1 disableable. + identity.add_public_key(authentication_key(4, SecurityLevel::CRITICAL)); + assert!(build( + &identity, + vec![], + vec![1], + &signer, + PlatformVersion::latest() + ) + .await + .is_ok()); + } + + #[tokio::test] + async fn should_count_a_hash160_key_as_remaining_in_the_builder() { + let mut identity = one_key_per_level(); + identity.add_public_key(key( + 4, + Purpose::AUTHENTICATION, + SecurityLevel::CRITICAL, + KeyType::ECDSA_HASH160, + )); + let signer = CountingSigner::default(); + let transition = build( + &identity, + vec![], + vec![1], + &signer, + PlatformVersion::latest(), + ) + .await + .expect("an ECDSA_HASH160 CRITICAL key keeps the CRITICAL level valid"); + assert_matches!(transition, StateTransition::IdentityUpdate(_)); + } + + /// The in-place edit of this shipped generation is inert for a request the rule accepts: + /// at every protocol version, the last shipped one included, the builder produces exactly + /// the transition it produced before the rule existed. + #[tokio::test] + async fn should_build_an_unchanged_transition_for_a_compliant_update_at_every_version() { + let identity = one_key_per_level(); + let added = authentication_key(4, SecurityLevel::HIGH); + let mut added_in_creation: IdentityPublicKeyInCreation = (&added).into(); + added_in_creation.set_signature(SIGNATURE.to_vec().into()); + let expected: StateTransition = IdentityUpdateTransitionV0 { + identity_id: identity.id(), + revision: identity.revision(), + nonce: 5, + add_public_keys: vec![added_in_creation], + // CRITICAL 1 stands in for HIGH 2. + disable_public_keys: vec![2], + user_fee_increase: 0, + signature_public_key_id: 0, + signature: SIGNATURE.to_vec().into(), + } + .into(); + + for platform_version in PLATFORM_VERSIONS { + let signer = CountingSigner::default(); + let transition = build( + &identity, + vec![added.clone()], + vec![2], + &signer, + platform_version, + ) + .await + .unwrap_or_else(|e| { + panic!( + "protocol version {}: {e}", + platform_version.protocol_version + ) + }); + assert_eq!( + transition, expected, + "protocol version {}", + platform_version.protocol_version + ); + } + } +} diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 0dbca35e9b1..8eac3d17cfb 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -684,6 +684,29 @@ pub async fn test_platform_wallet_manager() -> ( pub async fn test_platform_wallet_manager_with

( persister: Arc

, ) -> (Arc>, WalletId) +where + P: crate::changeset::PlatformWalletPersistence + 'static, +{ + let sdk = dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"); + platform_wallet_manager_over(sdk, persister).await +} + +/// [`test_platform_wallet_manager`] over a caller-built mock SDK, for tests that +/// set mock expectations (an identity nonce, say) before the manager takes it. +#[cfg(test)] +pub(crate) async fn test_platform_wallet_manager_with_sdk( + sdk: dash_sdk::Sdk, +) -> ( + Arc>, + WalletId, +) { + platform_wallet_manager_over(sdk, Arc::new(NoopTestPersister)).await +} + +async fn platform_wallet_manager_over

( + sdk: dash_sdk::Sdk, + persister: Arc

, +) -> (Arc>, WalletId) where P: crate::changeset::PlatformWalletPersistence + 'static, { @@ -694,7 +717,7 @@ where const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ abandon abandon abandon abandon abandon about"; - let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let sdk = Arc::new(sdk); let event_handler: Arc = Arc::new(NoopTestEventHandler); let manager = Arc::new(crate::PlatformWalletManager::new( diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs index b892a29a985..aa738e5b017 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs @@ -18,7 +18,6 @@ use dpp::ProtocolError; use dpp::identity::signer::Signer; use dash_sdk::platform::transition::put_settings::PutSettings; -use dash_sdk::platform::transition::update_identity::ensure_keys_can_be_disabled; use crate::error::PlatformWalletError; @@ -73,8 +72,9 @@ impl IdentityWallet { /// MASTER specifically — HIGH/CRITICAL aren't accepted). /// /// An update that would disable the last valid authentication key of a - /// security level is refused before anything is built or signed, with - /// `PlatformWalletError::Sdk(dash_sdk::Error::LastAuthenticationKey(_))` + /// security level is refused by the transition builder before anything is + /// signed or broadcast, with + /// `PlatformWalletError::Sdk(dash_sdk::Error::Protocol(ProtocolError::LastAuthenticationKey(_)))` /// (DIP-11); keys added in the same update count as valid. /// /// NOTE: callers that ADD keys via `add_public_keys` are @@ -136,9 +136,6 @@ impl IdentityWallet { (identity, index) }; - // DIP-11, before the nonce is spent or anything is signed. - ensure_keys_can_be_disabled(&identity, &add_public_keys, &disable_public_keys)?; - // Increment revision for the update transition. let original_revision = identity.revision(); identity.set_revision(original_revision + 1); @@ -187,11 +184,16 @@ impl IdentityWallet { None, ) .await - .map_err(|e| { - PlatformWalletError::InvalidIdentityData(format!( + .map_err(|e| match e { + // The builder's DIP-11 refusal stays typed, as on the + // caller-supplied-identity entry point. + refusal @ ProtocolError::LastAuthenticationKey(_) => { + PlatformWalletError::Sdk(dash_sdk::Error::Protocol(refusal)) + } + e => PlatformWalletError::InvalidIdentityData(format!( "Failed to create identity update transition: {}", e - )) + )), })?; state_transition @@ -293,8 +295,10 @@ impl IdentityWallet { /// `docs/protocol/authentication-key-limits.md`. /// /// An update that would disable the last valid authentication key of a - /// security level is refused before anything is built or signed, with - /// [`dash_sdk::Error::LastAuthenticationKey`] (DIP-11). + /// security level is refused by the transition builder before anything is + /// signed or broadcast, with + /// `dash_sdk::Error::Protocol(ProtocolError::LastAuthenticationKey(_))` + /// (DIP-11). /// /// Returns the [`StateTransitionProofResult`] from the broadcast so callers /// can inspect proof-verified outcomes (e.g. updated keys, balance). @@ -312,9 +316,6 @@ impl IdentityWallet { use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; - // DIP-11, before the nonce is spent or anything is signed. - ensure_keys_can_be_disabled(identity, &add_public_keys, &disable_public_keys)?; - // Get identity nonce from Platform. let identity_nonce = self .sdk @@ -352,8 +353,10 @@ impl IdentityWallet { #[cfg(test)] mod tests { use super::*; - use crate::test_support::test_platform_wallet_manager; + use crate::test_support::{test_platform_wallet_manager_with_sdk, NoopTestPersister}; use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + use crate::wallet::platform_wallet::WalletId; + use dash_sdk::query_types::IdentityNonceFetcher; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; use dpp::identity::v0::IdentityV0; use std::collections::BTreeMap; @@ -421,11 +424,27 @@ mod tests { .into() } - /// The mock SDK answers no request, so a refusal that reached the network - /// would surface as a different error. + /// A wallet over a mock SDK that answers only the identity-nonce query, so + /// an update that got past the builder would fail its broadcast with a + /// different error. + async fn manager_answering_only_the_nonce() -> ( + Arc>, + WalletId, + ) { + let mut sdk = dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"); + sdk.mock() + .expect_fetch::( + Identifier::from([1; 32]), + Some(IdentityNonceFetcher(0)), + ) + .await + .expect("nonce expectation"); + test_platform_wallet_manager_with_sdk(sdk).await + } + #[tokio::test] async fn should_refuse_disabling_the_last_critical_key_before_sending_anything() { - let (manager, wallet_id) = test_platform_wallet_manager().await; + let (manager, wallet_id) = manager_answering_only_the_nonce().await; let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); { let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); @@ -449,18 +468,19 @@ mod tests { assert!( matches!( result, - Err(PlatformWalletError::Sdk(dash_sdk::Error::LastAuthenticationKey(ref refusal))) + Err(PlatformWalletError::Sdk(dash_sdk::Error::Protocol( + ProtocolError::LastAuthenticationKey(ref refusal) + ))) if refusal.key_id == 1 && refusal.security_level == SecurityLevel::CRITICAL ), "{result:?}" ); } - /// The caller-supplied-identity entry point refuses the same way, before - /// the nonce is fetched. + /// The caller-supplied-identity entry point refuses the same way. #[tokio::test] async fn should_refuse_disabling_the_last_critical_key_with_a_supplied_identity() { - let (manager, wallet_id) = test_platform_wallet_manager().await; + let (manager, wallet_id) = manager_answering_only_the_nonce().await; let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); let result = wallet @@ -470,7 +490,7 @@ mod tests { assert!( matches!( result, - Err(dash_sdk::Error::LastAuthenticationKey(ref refusal)) + Err(dash_sdk::Error::Protocol(ProtocolError::LastAuthenticationKey(ref refusal))) if refusal.key_id == 1 && refusal.security_level == SecurityLevel::CRITICAL ), "{result:?}" diff --git a/packages/rs-sdk/src/error.rs b/packages/rs-sdk/src/error.rs index 8e2ece2ba68..d070903ac46 100644 --- a/packages/rs-sdk/src/error.rs +++ b/packages/rs-sdk/src/error.rs @@ -1,6 +1,5 @@ //! Definitions of errors use crate::platform::encrypted_for::EncryptedForError; -use crate::platform::transition::update_identity::LastAuthenticationKeyError; use dapi_grpc::platform::v0::StateTransitionBroadcastError as StateTransitionBroadcastErrorProto; use dapi_grpc::tonic::Code; pub use dash_context_provider::ContextProviderError; @@ -138,11 +137,6 @@ pub enum Error { #[error(transparent)] EncryptedFor(#[from] EncryptedForError), - /// An identity update would disable the last valid authentication key of a security - /// level, which DIP-11 forbids. Refused before anything was signed. - #[error(transparent)] - LastAuthenticationKey(#[from] LastAuthenticationKeyError), - /// The request uses a feature that not every node of the network serves yet, so the /// SDK refused it without sending it: nodes of an older release would answer /// UNIMPLEMENTED or a different question, and get banned for it. diff --git a/packages/rs-sdk/src/platform/transition.rs b/packages/rs-sdk/src/platform/transition.rs index 13c0dd2b127..3a99af47952 100644 --- a/packages/rs-sdk/src/platform/transition.rs +++ b/packages/rs-sdk/src/platform/transition.rs @@ -41,7 +41,6 @@ pub mod transfer_to_addresses; mod txid; #[cfg(feature = "shielded")] pub mod unshield; -pub mod update_identity; pub mod update_identity_key_limits; pub mod update_price_of_document; pub(crate) mod validation; diff --git a/packages/strategy-tests/src/transitions.rs b/packages/strategy-tests/src/transitions.rs index 751b4f28b89..2d531dd85d3 100644 --- a/packages/strategy-tests/src/transitions.rs +++ b/packages/strategy-tests/src/transitions.rs @@ -59,6 +59,7 @@ use dpp::native_bls::NativeBlsModule; use dpp::state_transition::identity_topup_transition::methods::IdentityTopUpTransitionMethodsV0; use dpp::state_transition::identity_topup_transition::IdentityTopUpTransition; use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; +use dpp::state_transition::identity_update_transition::v0::IdentityUpdateTransitionV0; use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; use dpp::state_transition::{GetDataContractSecurityLevelRequirementFn, StateTransition}; use dpp::version::PlatformVersion; @@ -571,12 +572,12 @@ pub async fn create_identity_update_transition_disable_keys( } }); - let master_key_id = *identity + let master_key = identity .public_keys() - .iter() - .find(|(_, key)| key.security_level() == MASTER) + .values() + .find(|key| key.security_level() == MASTER) .expect("expected to have a master key") - .0; + .clone(); let identity_nonce = identity_nonce_counter.entry(identity.id()).or_default(); *identity_nonce += 1; @@ -584,19 +585,38 @@ pub async fn create_identity_update_transition_disable_keys( // Bump revision before creating state transition - the transition uses identity.revision() directly identity.bump_revision(); - let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( - identity, - &master_key_id, - vec![], - key_ids_to_disable, - *identity_nonce, - 0, - signer, - platform_version, - None, - ) - .await - .expect("expected to create a DisableKeys transition"); + // Built by hand rather than through `IdentityUpdateTransition::try_from_identity_with_signer`: + // that builder applies DIP-11's client-side policy and refuses to disable the last key of a + // security level, which consensus accepts for every level but MASTER and these tests must + // keep exercising. The fields and signature match what the builder produces otherwise. + let serialization_version = platform_version + .dpp + .state_transition_serialization_versions + .identity_update_state_transition + .default_current_version; + assert_eq!( + serialization_version, 0, + "strategy tests build identity updates in serialization version 0 only" + ); + let mut state_transition: StateTransition = IdentityUpdateTransitionV0 { + signature: Default::default(), + signature_public_key_id: 0, + identity_id: identity.id(), + revision: identity.revision(), + nonce: *identity_nonce, + add_public_keys: vec![], + disable_public_keys: key_ids_to_disable, + user_fee_increase: 0, + } + .into(); + state_transition + .sign_external( + &master_key, + signer, + None::, + ) + .await + .expect("expected to sign a DisableKeys transition"); Some(state_transition) } @@ -1291,3 +1311,82 @@ pub async fn create_state_transitions_for_identities_and_proofs( } results } + +#[cfg(test)] +mod tests { + use super::*; + use dpp::state_transition::identity_update_transition::accessors::IdentityUpdateTransitionAccessorsV0; + use rand::SeedableRng; + + /// The hand-built DisableKeys transition is byte-for-byte what dpp's builder signs whenever + /// the builder's DIP-11 client policy lets the update through, so bypassing that policy + /// changes nothing else about the seeded runs. + #[tokio::test] + async fn should_build_the_dpp_builder_transition_whenever_the_builder_accepts_the_disable() { + let platform_version = PlatformVersion::latest(); + let mut compared = 0; + for seed in 0..32 { + let mut rng = StdRng::seed_from_u64(seed); + let (identity, private_keys): (Identity, BTreeMap) = + Identity::random_identity_with_main_keys_with_private_key( + 5, + &mut rng, + platform_version, + ) + .expect("expected a random identity"); + let mut signer = SimpleSigner::default(); + signer.add_identity_public_keys(private_keys); + + let mut updated = identity.clone(); + let Some(hand_built) = create_identity_update_transition_disable_keys( + &mut updated, + 1, + &mut BTreeMap::new(), + 1_000, + &mut signer, + &mut rng, + platform_version, + ) + .await + else { + continue; + }; + let StateTransition::IdentityUpdate(update) = &hand_built else { + panic!("expected an identity update, got {hand_built:?}"); + }; + + let master_key_id = *identity + .public_keys() + .iter() + .find(|(_, key)| key.security_level() == MASTER) + .expect("expected a master key") + .0; + let mut bumped = identity.clone(); + bumped.bump_revision(); + match IdentityUpdateTransition::try_from_identity_with_signer( + &bumped, + &master_key_id, + vec![], + update.public_key_ids_to_disable().to_vec(), + 1, + 0, + &signer, + platform_version, + None, + ) + .await + { + Ok(built) => { + assert_eq!(built, hand_built, "seed {seed}"); + compared += 1; + } + Err(ProtocolError::LastAuthenticationKey(_)) => {} + Err(e) => panic!("seed {seed}: {e}"), + } + } + assert!( + compared > 0, + "no seed produced an update the builder accepts" + ); + } +} diff --git a/packages/wasm-sdk/src/error.rs b/packages/wasm-sdk/src/error.rs index 4bae134263a..218a322b86e 100644 --- a/packages/wasm-sdk/src/error.rs +++ b/packages/wasm-sdk/src/error.rs @@ -284,13 +284,6 @@ impl From for WasmSdkError { retriable, ), EncryptedFor(e) => e.into(), - // A caller's request DIP-11 forbids: retrying cannot change it. - LastAuthenticationKey(e) => Self::new( - WasmSdkErrorKind::InvalidArgument, - e.to_string(), - None, - false, - ), error @ ProtocolVersionNotActive { .. } => { // The SDK's message names what is missing; a failed query for the network's // version is its source, and JS sees only the message. The refusal holds @@ -325,8 +318,14 @@ impl From for WasmSdkError { } impl From for WasmSdkError { fn from(err: ProtocolError) -> Self { + // A DIP-11 refusal judges the caller's request, not the protocol: retrying cannot + // change it. + let kind = match err { + ProtocolError::LastAuthenticationKey(_) => WasmSdkErrorKind::InvalidArgument, + _ => WasmSdkErrorKind::Protocol, + }; let code = consensus_error_code(&err); - Self::new(WasmSdkErrorKind::Protocol, err.to_string(), code, false) + Self::new(kind, err.to_string(), code, false) } } @@ -442,6 +441,8 @@ mod tests { use dash_sdk::dpp::consensus::state::contract_moderation::ContractUserBannedError; use dash_sdk::dpp::consensus::ConsensusError; use dash_sdk::dpp::data_contract::errors::DataContractError; + use dash_sdk::dpp::identity::key_disable_rule::LastAuthenticationKeyError; + use dash_sdk::dpp::identity::SecurityLevel; use dash_sdk::dpp::platform_value::Identifier; use dash_sdk::dpp::serialization::PlatformSerializableWithPlatformVersion; use dash_sdk::dpp::version::PlatformVersion; @@ -648,6 +649,24 @@ mod tests { assert_eq!(error.code(), 10231); } + #[test] + fn should_report_a_dip11_last_key_refusal_as_an_invalid_argument() { + let refusal = ProtocolError::from(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }); + let message = refusal.to_string(); + let error = WasmSdkError::with_context("Failed to create update transition", refusal); + + assert_eq!(error.kind(), WasmSdkErrorKind::InvalidArgument); + assert_eq!(error.code(), -1); + assert!(!error.is_retriable()); + assert_eq!( + error.message(), + format!("Failed to create update transition: {message}") + ); + } + #[test] fn should_report_no_code_for_a_protocol_error_that_is_not_a_consensus_error() { let error = WasmSdkError::from(ProtocolError::Generic("not consensus".to_string())); diff --git a/packages/wasm-sdk/src/state_transitions/identity.rs b/packages/wasm-sdk/src/state_transitions/identity.rs index 32e1b4852d5..988f305bd11 100644 --- a/packages/wasm-sdk/src/state_transitions/identity.rs +++ b/packages/wasm-sdk/src/state_transitions/identity.rs @@ -20,7 +20,6 @@ use dash_sdk::dpp::voting::votes::Vote; use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; use dash_sdk::platform::transition::put_identity::PutIdentity; use dash_sdk::platform::transition::top_up_identity::TopUpIdentity; -use dash_sdk::platform::transition::update_identity::ensure_keys_can_be_disabled; use dash_sdk::platform::transition::update_identity_key_limits::{ raised_key_limits, UpdateIdentityKeyLimits, }; @@ -586,10 +585,10 @@ impl WasmSdk { /// /// This method handles the complete update flow: /// 1. Validates the master key for signing - /// 2. Refuses to disable the last valid authentication key of a security level - /// (DIP-11); keys added in the same update count - /// 3. Builds and signs the identity update transition - /// 4. Broadcasts and waits for confirmation + /// 2. Builds and signs the identity update transition; the builder refuses, before + /// signing, to disable the last valid authentication key of a security level + /// (DIP-11), counting keys added in the same update + /// 3. Broadcasts and waits for confirmation /// /// @param options - Update options including identity, keys to add/disable, and signer /// @returns Promise that resolves when the update is complete @@ -676,9 +675,6 @@ impl WasmSdk { // Get keys to disable let keys_to_disable = parsed.disable_public_keys.unwrap_or_default(); - // DIP-11, before the nonce is spent or anything is signed. - ensure_keys_can_be_disabled(&identity, &keys_to_add, &keys_to_disable)?; - // Get identity nonce let identity_nonce = self .inner_sdk() From 26aacc99c01f31daac1e215ea82b5a5178a41bae Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:14:51 +0000 Subject: [PATCH 10/15] fix(dpp): count only unbudgeted keys with matching contract bounds as DIP-11 stand-ins A key with a total_budget no longer keeps another key of its level disableable: what is left of the budget lives in Drive, and an exhausted key cannot sign. A contract-bound key no longer stands in for an unbounded one; a bound key is replaced by an unbounded key or one with identical bounds. The MASTER comments now say it signs identity and key-limits updates. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG --- .../rs-dpp/src/identity/key_disable_rule.rs | 136 +++++++++++++++--- 1 file changed, 118 insertions(+), 18 deletions(-) diff --git a/packages/rs-dpp/src/identity/key_disable_rule.rs b/packages/rs-dpp/src/identity/key_disable_rule.rs index 2332ad773ab..5952f485908 100644 --- a/packages/rs-dpp/src/identity/key_disable_rule.rs +++ b/packages/rs-dpp/src/identity/key_disable_rule.rs @@ -22,8 +22,8 @@ use crate::prelude::TimestampMillis; /// update for every level but MASTER. /// /// A security level is a minimum: a key of a stronger level stands in for it, except MASTER, -/// which signs identity updates only. So a CRITICAL key keeps a HIGH key disableable, a MASTER -/// key does not, and a MASTER key needs another MASTER key. +/// which signs identity updates and key-limits updates only. So a CRITICAL key keeps a HIGH key +/// disableable, a MASTER key does not, and a MASTER key needs another MASTER key. #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] #[error( "cannot disable authentication key {key_id}: no other enabled, unexpired authentication key \ @@ -42,12 +42,19 @@ pub struct LastAuthenticationKeyError { /// /// A valid key is an enabled authentication key that has not expired, whatever its key type: /// `ECDSA_HASH160` keys, which identities created by older Dash Evo Tool versions carry, count. -/// The keys the identity keeps count, and so do the keys the same update adds. +/// The keys the identity keeps count, and so do the keys the same update adds. A key stands in +/// for the disabled one only where it can sign everything the disabled one could: +/// +/// - **Budget.** A key with a `total_budget` never counts: what is left of the budget lives in +/// Drive, and an exhausted key can no longer sign. +/// - **Contract bounds.** An unbounded key is replaced only by an unbounded key; a bound key by +/// an unbounded key or one with identical bounds. Platform refuses a bound key outside its +/// bounds. /// /// `now_ms` is the caller's current time in Unix milliseconds. dpp reads no clock, so `None` -/// judges conservatively: a key with any `expires_at` does not count as remaining, which is -/// stricter than DIP-11 and therefore still compliant. With `Some(now)`, a key counts until -/// `now` reaches its `expires_at`. +/// judges conservatively: a key with any `expires_at` does not count as remaining. With +/// `Some(now)`, a key counts until `now` reaches its `expires_at`. Each of these is stricter +/// than DIP-11 and therefore still compliant. /// /// Ids the identity does not hold, and keys of other purposes, are not judged here: Platform /// refuses unknown ids, and DIP-11 speaks of authentication keys. @@ -66,6 +73,7 @@ pub fn check_keys_can_be_disabled( .filter(|key| { key.purpose() == Purpose::AUTHENTICATION && key.disabled_at().is_none() + && key.total_budget().is_none() && is_unexpired(key, now_ms) }) }; @@ -76,13 +84,10 @@ pub fn check_keys_can_be_disabled( if key.purpose() != Purpose::AUTHENTICATION { continue; } - let security_level = key.security_level(); - if !remaining_valid_keys() - .any(|remaining| stands_in_for(remaining.security_level(), security_level)) - { + if !remaining_valid_keys().any(|remaining| stands_in_for(remaining, key)) { return Err(LastAuthenticationKeyError { key_id: *key_id, - security_level, + security_level: key.security_level(), }); } } @@ -97,18 +102,27 @@ fn is_unexpired(key: &IdentityPublicKey, now_ms: Option) -> boo } } -/// Whether a key at `candidate` satisfies a requirement for `required`: the same level, or a -/// stronger one other than MASTER, which signs nothing but identity updates. The single place -/// the level comparison of this rule is made. -fn stands_in_for(candidate: SecurityLevel, required: SecurityLevel) -> bool { - candidate == required - || (candidate != SecurityLevel::MASTER && candidate.stronger_security_than(required)) +/// Whether `candidate` can sign whatever `disabled` could, as far as level and contract bounds +/// go: the same level, or a stronger one other than MASTER, which signs identity updates and +/// key-limits updates only; and no bounds, or the same bounds as `disabled`. The single place +/// these comparisons of the rule are made. +fn stands_in_for(candidate: &IdentityPublicKey, disabled: &IdentityPublicKey) -> bool { + let (candidate_level, required_level) = (candidate.security_level(), disabled.security_level()); + let level_suffices = candidate_level == required_level + || (candidate_level != SecurityLevel::MASTER + && candidate_level.stronger_security_than(required_level)); + let bounds_suffice = match candidate.contract_bounds() { + None => true, + bounds => bounds == disabled.contract_bounds(), + }; + level_suffices && bounds_suffice } #[cfg(test)] pub(crate) mod tests { use super::*; use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; + use crate::identity::identity_public_key::contract_bounds::ContractBounds; use crate::identity::identity_public_key::v0::IdentityPublicKeyV0; use crate::identity::v0::IdentityV0; use crate::identity::KeyType; @@ -209,7 +223,7 @@ pub(crate) mod tests { #[test] fn should_not_let_a_master_key_stand_in_for_critical() { - // MASTER signs identity updates only, so it cannot replace the CRITICAL key. + // MASTER signs identity and key-limits updates only, so it cannot replace the CRITICAL key. let identity = identity(vec![ authentication_key(0, SecurityLevel::MASTER), authentication_key(1, SecurityLevel::CRITICAL), @@ -305,6 +319,92 @@ pub(crate) mod tests { ); } + #[test] + fn should_not_count_a_key_with_a_budget_as_remaining() { + // What is left of a budget lives in Drive: an exhausted budgeted key can no longer + // sign, and the rule cannot tell it from a fresh one. + let budgeted = authentication_key(4, SecurityLevel::CRITICAL).with_limits(Some(1), None); + let mut identity = one_key_per_level(); + identity.add_public_key(budgeted.clone()); + for now_ms in [Some(NOW_MS), None] { + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], now_ms), + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + assert!(check_keys_can_be_disabled( + &one_key_per_level(), + &[budgeted.clone()], + &[1], + now_ms + ) + .is_err()); + } + // An unbudgeted replacement added in the same update still counts. + let replacement = authentication_key(5, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&identity, &[replacement], &[1], Some(NOW_MS)), + Ok(()) + ); + } + + fn bound_to(key: IdentityPublicKey, contract: u8) -> IdentityPublicKey { + let IdentityPublicKey::V0(mut v0) = key else { + panic!("test keys are V0"); + }; + v0.contract_bounds = Some(ContractBounds::SingleContract { + id: Identifier::from([contract; 32]), + }); + v0.into() + } + + #[test] + fn should_not_let_a_contract_bound_key_stand_in_for_an_unbounded_one() { + // A bound key signs only inside its contract, so it cannot do the unbounded key's work. + let bound = bound_to(authentication_key(4, SecurityLevel::CRITICAL), 7); + let mut identity = one_key_per_level(); + identity.add_public_key(bound.clone()); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)), + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + assert!( + check_keys_can_be_disabled(&one_key_per_level(), &[bound], &[1], Some(NOW_MS)).is_err() + ); + } + + #[test] + fn should_let_an_unbounded_or_identically_bound_key_stand_in_for_a_bound_one() { + let bound = bound_to(authentication_key(4, SecurityLevel::CRITICAL), 7); + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + bound.clone(), + ]); + let unbounded = authentication_key(5, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&identity, &[unbounded], &[4], Some(NOW_MS)), + Ok(()) + ); + let same_bounds = bound_to(authentication_key(5, SecurityLevel::CRITICAL), 7); + assert_eq!( + check_keys_can_be_disabled(&identity, &[same_bounds], &[4], Some(NOW_MS)), + Ok(()) + ); + let other_bounds = bound_to(authentication_key(5, SecurityLevel::CRITICAL), 8); + assert_eq!( + check_keys_can_be_disabled(&identity, &[other_bounds], &[4], Some(NOW_MS)), + Err(LastAuthenticationKeyError { + key_id: 4, + security_level: SecurityLevel::CRITICAL, + }) + ); + } + #[test] fn should_not_judge_keys_of_other_purposes_or_unknown_ids() { let identity = identity(vec![ From be4ae6741b891141effb67653590d799f2210512 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:25:34 +0000 Subject: [PATCH 11/15] fix(sdk): refuse disabled sender and recipient keys when creating a contact request DIP-15 requires both key indices of a new contact request to name a valid key. create_contact_request checked presence and purpose only, so a direct SDK caller could encrypt the payment xpub to a revoked recipient key. The key checks now live in one helper that also refuses a disabled key on either side, before any ECDH. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG --- .../src/platform/dashpay/contact_request.rs | 185 ++++++++++++++---- 1 file changed, 142 insertions(+), 43 deletions(-) diff --git a/packages/rs-sdk/src/platform/dashpay/contact_request.rs b/packages/rs-sdk/src/platform/dashpay/contact_request.rs index 88cdad40d96..45bcbbd66a5 100644 --- a/packages/rs-sdk/src/platform/dashpay/contact_request.rs +++ b/packages/rs-sdk/src/platform/dashpay/contact_request.rs @@ -176,6 +176,76 @@ pub fn recipient_key_purpose_is_valid(purpose: Purpose) -> bool { matches!(purpose, Purpose::DECRYPTION | Purpose::ENCRYPTION) } +/// The sender's and the recipient's keys a contact request **we are about to +/// mint** names, once both pass DIP-15's send-side checks: each exists, has a +/// purpose [`sender_key_purpose_is_valid`] / [`recipient_key_purpose_is_valid`] +/// accepts, and is valid, i.e. not disabled. Encrypting the payment xpub to a +/// disabled key would hand it to whoever holds the revoked private half. +/// +/// The single owner of these checks. Received documents are not judged here: a +/// `contactRequest` is immutable, and one naming a key disabled since it was +/// sent stays usable (DIP-11). +fn contact_request_keys<'a>( + sender_identity: &'a Identity, + sender_key_index: u32, + recipient_identity: &'a Identity, + recipient_key_index: u32, +) -> Result<(&'a IdentityPublicKey, &'a IdentityPublicKey), Error> { + let sender_key = sender_identity + .public_keys() + .get(&sender_key_index) + .ok_or_else(|| { + Error::Generic(format!( + "Sender identity does not have encryption key at index {}", + sender_key_index + )) + })?; + + // Sender always references its own ENCRYPTION key (the live + // convention of both on-chain cohorts). + if !sender_key_purpose_is_valid(sender_key.purpose()) { + return Err(Error::Generic(format!( + "Sender key at index {} is not an encryption key", + sender_key_index + ))); + } + + let recipient_key = recipient_identity + .public_keys() + .get(&recipient_key_index) + .ok_or_else(|| { + Error::Generic(format!( + "Recipient identity does not have a key at index {}", + recipient_key_index + )) + })?; + + // Accept either a DECRYPTION key (newest cohort / our original + // convention) OR an ENCRYPTION key (the dominant mobile cohort, whose + // identities carry no DECRYPTION key and reference their ENCRYPTION + // key for recipientKeyIndex). + if !recipient_key_purpose_is_valid(recipient_key.purpose()) { + return Err(Error::Generic(format!( + "Recipient key at index {} is not a decryption or encryption key", + recipient_key_index + ))); + } + + for (side, key_index, key) in [ + ("Sender", sender_key_index, sender_key), + ("Recipient", recipient_key_index, recipient_key), + ] { + if let Some(disabled_at) = key.disabled_at() { + return Err(Error::Generic(format!( + "{side} key at index {key_index} is disabled (at timestamp {disabled_at}); a new \ + contact request needs a valid key" + ))); + } + } + + Ok((sender_key, recipient_key)) +} + impl Sdk { /// Create a contact request document /// @@ -203,7 +273,8 @@ impl Sdk { /// Returns an error if: /// - The DashPay contract cannot be fetched /// - The contactRequest document type is not found - /// - The sender or recipient doesn't have the required encryption keys + /// - The sender or recipient doesn't have the required encryption keys, or either key is + /// disabled /// - ECDH encryption fails /// - The shared secret, private key, or extended public key cannot be retrieved pub async fn create_contact_request( @@ -241,48 +312,12 @@ impl Sdk { } }; - // Verify sender has the encryption key at the specified index - let sender_key = input - .sender_identity - .public_keys() - .get(&input.sender_key_index) - .ok_or_else(|| { - Error::Generic(format!( - "Sender identity does not have encryption key at index {}", - input.sender_key_index - )) - })?; - - // Sender always references its own ENCRYPTION key (the live - // convention of both on-chain cohorts). - if !sender_key_purpose_is_valid(sender_key.purpose()) { - return Err(Error::Generic(format!( - "Sender key at index {} is not an encryption key", - input.sender_key_index - ))); - } - - // Verify recipient has the referenced key at the specified index. - let recipient_key = recipient_identity - .public_keys() - .get(&input.recipient_key_index) - .ok_or_else(|| { - Error::Generic(format!( - "Recipient identity does not have a key at index {}", - input.recipient_key_index - )) - })?; - - // Accept either a DECRYPTION key (newest cohort / our original - // convention) OR an ENCRYPTION key (the dominant mobile cohort, whose - // identities carry no DECRYPTION key and reference their ENCRYPTION - // key for recipientKeyIndex). - if !recipient_key_purpose_is_valid(recipient_key.purpose()) { - return Err(Error::Generic(format!( - "Recipient key at index {} is not a decryption or encryption key", - input.recipient_key_index - ))); - } + let (sender_key, recipient_key) = contact_request_keys( + &input.sender_identity, + input.sender_key_index, + &recipient_identity, + input.recipient_key_index, + )?; // Get the recipient's public key data for ECDH let recipient_public_key_data = recipient_key.data(); @@ -677,6 +712,70 @@ mod tests { assert!(!sender_key_purpose_is_valid(Purpose::AUTHENTICATION)); } + fn identity_with_key(purpose: Purpose, disabled_at: Option) -> Identity { + use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dpp::identity::v0::IdentityV0; + use dpp::identity::{KeyType, SecurityLevel}; + use dpp::platform_value::BinaryData; + let (_, public_key) = secp256k1::generate_keypair(&mut rand::rng()); + let key = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: 0, + purpose, + security_level: SecurityLevel::MEDIUM, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new(public_key.serialize().to_vec()), + disabled_at, + }); + IdentityV0 { + id: Identifier::from([2; 32]), + public_keys: BTreeMap::from([(0, key)]), + balance: 0, + revision: 0, + } + .into() + } + + #[test] + fn should_accept_enabled_sender_and_recipient_keys() { + let sender = identity_with_key(Purpose::ENCRYPTION, None); + let recipient = identity_with_key(Purpose::DECRYPTION, None); + let (sender_key, recipient_key) = + contact_request_keys(&sender, 0, &recipient, 0).expect("both keys are valid"); + assert_eq!(sender_key, sender.public_keys().get(&0).unwrap()); + assert_eq!(recipient_key, recipient.public_keys().get(&0).unwrap()); + } + + #[test] + fn should_refuse_a_disabled_sender_key() { + let sender = identity_with_key(Purpose::ENCRYPTION, Some(1_700_000_000_000)); + let recipient = identity_with_key(Purpose::DECRYPTION, None); + let error = contact_request_keys(&sender, 0, &recipient, 0) + .expect_err("a disabled sender key must be refused"); + assert!( + error + .to_string() + .contains("Sender key at index 0 is disabled"), + "{error}" + ); + } + + #[test] + fn should_refuse_a_disabled_recipient_key() { + // Encrypting the payment xpub to a revoked key hands it to whoever holds the key. + let sender = identity_with_key(Purpose::ENCRYPTION, None); + let recipient = identity_with_key(Purpose::ENCRYPTION, Some(1_700_000_000_000)); + let error = contact_request_keys(&sender, 0, &recipient, 0) + .expect_err("a disabled recipient key must be refused"); + assert!( + error + .to_string() + .contains("Recipient key at index 0 is disabled"), + "{error}" + ); + } + #[test] fn test_ecdh_shared_secret_symmetry() { // Test that both parties derive the same shared secret From 11f4dbcc4a49ab9fb46bc8fa2e06e1e44dab06dd Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:30:33 +0000 Subject: [PATCH 12/15] refactor(platform-wallet): leave the send-side disabled-key refusal to the SDK Sdk::create_contact_request now refuses a disabled sender or recipient key, so validate_contact_request_to_send duplicated it. The wallet's send path keeps the key-type gate of validate_contact_request and its enabled-only key selectors; the DIP-15 valid-key rule lives once, in the SDK. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG --- .../src/wallet/identity/crypto/validation.rs | 96 +------------------ .../identity/network/contact_requests.rs | 32 +++---- 2 files changed, 20 insertions(+), 108 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs index a661d45f951..0e4ac9507ff 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -1,9 +1,9 @@ //! Key validation for DashPay contact requests. //! //! [`validate_contact_request`] judges a request already on chain: liberal, -//! because the document is immutable. `validate_contact_request_to_send` -//! adds what DIP-15 asks of a request we are about to mint: both keys valid, -//! i.e. not disabled. +//! because the document is immutable. What DIP-15 asks of a request we are +//! about to mint on top of it (both keys valid, i.e. not disabled) is +//! enforced once, by `dash_sdk::Sdk::create_contact_request`. use dpp::identity::accessors::IdentityGettersV0; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; @@ -128,7 +128,7 @@ impl ContactRequestValidation { /// disabled since the request was sent is only a warning: DIP-11 keeps /// disabled keys usable for verification, and DIP-15's "valid key" rule /// binds the sender when minting, which -/// `validate_contact_request_to_send` enforces for our own requests. +/// `dash_sdk::Sdk::create_contact_request` enforces for our own requests. /// /// Three live cohorts are known: /// - **Newest** — bound `ENCRYPTION(sender)` / `DECRYPTION(recipient)`, our @@ -175,47 +175,6 @@ pub fn validate_contact_request( validation } -/// [`validate_contact_request`] for a request we are about to mint, plus -/// DIP-15's requirement that both key indices name a *valid* key: a disabled -/// key on either side is a hard error. Encrypting to a revoked key would hand -/// the payment xpub to whoever holds its private half. -pub(crate) fn validate_contact_request_to_send( - sender_identity: &Identity, - sender_key_index: u32, - recipient_identity: &Identity, - recipient_key_index: u32, -) -> ContactRequestValidation { - let mut validation = validate_contact_request( - sender_identity, - sender_key_index, - recipient_identity, - recipient_key_index, - ); - for (side, identity, key_index) in [ - ( - ContactRequestKeySide::Sender, - sender_identity, - sender_key_index, - ), - ( - ContactRequestKeySide::Recipient, - recipient_identity, - recipient_key_index, - ), - ] { - if let Some(disabled_at) = identity - .get_public_key_by_id(key_index) - .and_then(|key| key.disabled_at()) - { - validation.add_error(format!( - "{side} key {key_index} is disabled (at timestamp {disabled_at}); a new \ - contact request needs a valid key", - )); - } - } - validation -} - /// The sender half of [`validate_contact_request`] — the checks that need the /// **counterparty's** identity. /// @@ -557,53 +516,6 @@ mod tests { assert!(!recipient_half.is_permanent()); } - /// DIP-15 requires a valid key when SENDING: neither side may be disabled. - #[test] - fn should_refuse_to_send_with_a_disabled_key() { - let enabled_sender = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::ENCRYPTION, - )]); - let enabled_recipient = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::DECRYPTION, - )]); - let disabled_sender = make_identity(vec![disabled_key(0, Purpose::ENCRYPTION)]); - let disabled_recipient = make_identity(vec![disabled_key(0, Purpose::DECRYPTION)]); - - let valid = validate_contact_request_to_send(&enabled_sender, 0, &enabled_recipient, 0); - assert!(valid.is_valid, "errors: {:?}", valid.errors); - - for (sender, recipient, side) in [ - ( - &disabled_sender, - &enabled_recipient, - ContactRequestKeySide::Sender, - ), - ( - &enabled_sender, - &disabled_recipient, - ContactRequestKeySide::Recipient, - ), - ] { - let result = validate_contact_request_to_send(sender, 0, recipient, 0); - assert!( - !result.is_valid, - "a disabled {side} key must refuse the send" - ); - assert!( - result - .errors - .iter() - .any(|e| e.starts_with(&side.to_string()) && e.contains("disabled")), - "errors: {:?}", - result.errors - ); - } - } - #[test] fn test_merge() { let mut a = ContactRequestValidation::new(); diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index 7eabc52309e..b4fefc00d4a 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -18,7 +18,7 @@ use super::sdk_writer::SendContactRequestParams; use super::*; use crate::broadcaster::TransactionBroadcaster; use crate::error::PlatformWalletError; -use crate::wallet::identity::crypto::validation::validate_contact_request_to_send; +use crate::wallet::identity::crypto::validation::validate_contact_request; use crate::wallet::identity::types::dashpay::contact_request::ContactRequest; use crate::wallet::identity::types::dashpay::established_contact::EstablishedContact; use crate::wallet::identity::ManagedIdentity; @@ -468,7 +468,7 @@ impl DashPayView<'_, B> { // required for ECDH. Shared selector — same enabled-only policy // as the contactInfo publish path, so both DashPay surfaces // agree on the ECDH root and a disabled (rotated-away) first - // key can't hard-fail the pre-send validator below while an + // key can't hard-fail the SDK's disabled-key refusal while an // enabled replacement exists. let sender_encryption_key = select_own_encryption_key(&sender_identity)?.clone(); let sender_key_index = sender_encryption_key.id(); @@ -476,13 +476,13 @@ impl DashPayView<'_, B> { let recipient_key_index = select_recipient_key_index(&recipient_identity)?; // 3b. Gate the selected key pair through the receive validator - // plus DIP-15's send-side rule (neither key disabled), BEFORE - // any ECDH or broadcast. The selectors above pick plausible - // indices; the validator pins the full contract (key types, - // not disabled) so a malformed identity can't reach the - // encrypt-and-broadcast stage with a key that would poison - // the channel. - let validation = validate_contact_request_to_send( + // BEFORE any ECDH or broadcast. The selectors above pick + // plausible indices; the validator pins the key types so a + // malformed identity can't reach the encrypt-and-broadcast + // stage with a key that would poison the channel. DIP-15's + // send-side rule (neither key disabled) is enforced by + // `Sdk::create_contact_request`, its single owner. + let validation = validate_contact_request( &sender_identity, sender_key_index, &recipient_identity, @@ -1208,7 +1208,7 @@ fn select_recipient_key_index(recipient_identity: &Identity) -> Result = recipient_identity .public_keys() .iter() @@ -1236,10 +1236,10 @@ fn select_recipient_key_index(recipient_identity: &Identity) -> Result Result<&IdentityPublicKey, PlatformWalletError> { @@ -5760,8 +5760,8 @@ mod recipient_key_selection_tests { /// **Own-key selector: a disabled first ENCRYPTION key is skipped in /// favour of the enabled replacement.** After a disable-and-replace key /// rotation the lowest-id ENCRYPTION key is disabled; selecting it - /// would hard-fail the pre-send validator on every new outgoing contact - /// request ("Sender key N is disabled") even though an enabled + /// would fail every new outgoing contact request in the SDK + /// ("Sender key at index N is disabled") even though an enabled /// replacement exists — while the contactInfo path (which already /// filtered disabled keys) would use the replacement, splitting the two /// surfaces' notion of the ECDH root. Was red against the send path's From c784766554d1950eca0f7712535d7a4c8d4fe1ca Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:30:33 +0000 Subject: [PATCH 13/15] fix(platform-wallet-ffi): report the DIP-11 last-key refusal as an invalid parameter An identity update refused with ProtocolError::LastAuthenticationKey fell through to ErrorUnknown, so hosts could not tell the expected refusal from an internal failure. It now maps to ErrorInvalidParameter, like the other caller-argument rejections. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG --- packages/rs-platform-wallet-ffi/src/error.rs | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 0bc67f532c9..647a35ea910 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -1078,6 +1078,13 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::InvalidParameter(..) => { PlatformWalletFFIResultCode::ErrorInvalidParameter } + // An identity update that would disable the last valid + // authentication key of a security level (DIP-11), refused by the + // dpp builder before anything is signed: the caller picked keys it + // may not disable, the same class as the arm above. + PlatformWalletError::Sdk(dash_sdk::Error::Protocol( + dpp::ProtocolError::LastAuthenticationKey(_), + )) => PlatformWalletFFIResultCode::ErrorInvalidParameter, // A second producer of code 31 (the arm above is the first), // reached without any marker inspection at this layer: message // signing found no signable account for the address, or the signer @@ -2016,6 +2023,30 @@ mod tests { ); } + /// The DIP-11 last-key refusal of an identity update is a caller-argument + /// error, not an internal failure: hosts branch on it to say which key + /// cannot be disabled instead of showing ErrorUnknown. + #[test] + fn last_authentication_key_refusal_maps_to_invalid_parameter() { + use dpp::identity::key_disable_rule::LastAuthenticationKeyError; + use dpp::identity::SecurityLevel; + let err = PlatformWalletError::Sdk(dash_sdk::Error::Protocol( + dpp::ProtocolError::LastAuthenticationKey(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }), + )); + let result: PlatformWalletFFIResult = err.into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorInvalidParameter + ); + let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_string_lossy() + .into_owned(); + assert!(msg.contains("cannot disable authentication key 1"), "{msg}"); + } + /// The four DPNS-marketplace trade rejections each map to their own /// dedicated code rather than flattening to `ErrorUnknown`, and the /// not-found case rides the existing `NotFound`. Hosts branch on these From dfd660dcf7334438c07614f6a782986e9d7cbeae Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:41:51 +0000 Subject: [PATCH 14/15] test(dpp): borrow the budgeted key as a slice instead of cloning it Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG --- packages/rs-dpp/src/identity/key_disable_rule.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rs-dpp/src/identity/key_disable_rule.rs b/packages/rs-dpp/src/identity/key_disable_rule.rs index 5952f485908..15fc7c4263a 100644 --- a/packages/rs-dpp/src/identity/key_disable_rule.rs +++ b/packages/rs-dpp/src/identity/key_disable_rule.rs @@ -336,7 +336,7 @@ pub(crate) mod tests { ); assert!(check_keys_can_be_disabled( &one_key_per_level(), - &[budgeted.clone()], + std::slice::from_ref(&budgeted), &[1], now_ms ) From 1001ddaae91a720d1ff6b3a4f186348f840c26a8 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:37:34 +0000 Subject: [PATCH 15/15] fix(dpp): count only key types that can sign as DIP-11 replacements check_keys_can_be_disabled counted any enabled authentication key as a stand-in, whatever its key type, so an identity could disable its only signable CRITICAL key while keeping only a BIP13_SCRIPT_HASH or EDDSA_25519_HASH160 one, which neither StateTransition::sign nor Drive accepts for identity-signed transitions. Add KeyType::can_sign_state_transitions, an exhaustive match mirroring drive-abci's SUPPORTED_KEY_TYPES (ECDSA_SECP256K1, BLS12_381, ECDSA_HASH160), and require it of remaining and added keys. ECDSA_HASH160 still counts for Dash Evo Tool identities. Consensus is untouched. Also correct two docs: the wasm-sdk disablePublicKeys TS doc now states the rule the builder enforces, and INTEROP_DESK_CHECK.md describes the receive-side contact request validation as implemented (key type gate only, no purpose check). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG --- docs/dashpay/INTEROP_DESK_CHECK.md | 19 ++++---- .../identity/identity_public_key/key_type.rs | 15 +++++++ .../rs-dpp/src/identity/key_disable_rule.rs | 43 +++++++++++++++++-- .../src/state_transitions/identity.rs | 7 ++- 4 files changed, 70 insertions(+), 14 deletions(-) diff --git a/docs/dashpay/INTEROP_DESK_CHECK.md b/docs/dashpay/INTEROP_DESK_CHECK.md index 4a42145d843..ab67a726b67 100644 --- a/docs/dashpay/INTEROP_DESK_CHECK.md +++ b/docs/dashpay/INTEROP_DESK_CHECK.md @@ -471,17 +471,18 @@ The alignment recommendation above is superseded on the receive side by contacts established through the legacy Android/dashj client had 27 inbound requests that reference the recipient's AUTHENTICATION or TRANSFER key, and the documents are immutable, so rejecting them left those contacts unpayable. The -rules now are (rs-sdk `platform/dashpay/contact_request.rs`): +rules now are: -- **Send (documents we create):** unchanged. The sender key must be ENCRYPTION - (`sender_key_purpose_is_valid`) and the recipient key DECRYPTION or ENCRYPTION +- **Send (documents we create; rs-sdk `platform/dashpay/contact_request.rs`):** + unchanged. The sender key must be ENCRYPTION (`sender_key_purpose_is_valid`) + and the recipient key DECRYPTION or ENCRYPTION (`recipient_key_purpose_is_valid`). -- **Receive (documents already on chain):** the recipient key may be DECRYPTION, - ENCRYPTION, AUTHENTICATION or TRANSFER - (`recipient_key_purpose_is_acceptable_on_receive`), and the sender key - ENCRYPTION or AUTHENTICATION (`sender_key_purpose_is_acceptable_on_receive`). - Any other purpose is a mismatch that is skipped and retried, never a - permanently broken channel. +- **Receive (documents already on chain; rs-platform-wallet + `wallet/identity/crypto/validation.rs`, `validate_contact_request`):** key + purpose is not checked. Only the key type is: the sender key must be + ECDSA_SECP256K1, and the recipient key ECDSA_SECP256K1 or ECDSA_HASH160. A + key of another type is a permanent error; a key id absent from the identity + is retryable, since identities gain keys; a disabled key is only a warning. ## Re-checking against the reference clients diff --git a/packages/rs-dpp/src/identity/identity_public_key/key_type.rs b/packages/rs-dpp/src/identity/identity_public_key/key_type.rs index 02df74d6347..3747497cf3f 100644 --- a/packages/rs-dpp/src/identity/identity_public_key/key_type.rs +++ b/packages/rs-dpp/src/identity/identity_public_key/key_type.rs @@ -99,6 +99,21 @@ impl KeyType { } } + /// Can a key of this type sign an identity-signed state transition? + /// + /// Mirrors the `SUPPORTED_KEY_TYPES` set drive-abci checks in + /// `validate_state_transition_identity_signed` (ECDSA_SECP256K1, BLS12_381 and + /// ECDSA_HASH160); `StateTransition::sign` refuses the other types. Keep the two in step. + pub fn can_sign_state_transitions(&self) -> bool { + match self { + KeyType::ECDSA_SECP256K1 => true, + KeyType::BLS12_381 => true, + KeyType::ECDSA_HASH160 => true, + KeyType::BIP13_SCRIPT_HASH => false, + KeyType::EDDSA_25519_HASH160 => false, + } + } + /// Can this key type be understood as an address on the Core chain? pub fn is_core_address_key_type(&self) -> bool { match self { diff --git a/packages/rs-dpp/src/identity/key_disable_rule.rs b/packages/rs-dpp/src/identity/key_disable_rule.rs index 15fc7c4263a..ebec25c36ac 100644 --- a/packages/rs-dpp/src/identity/key_disable_rule.rs +++ b/packages/rs-dpp/src/identity/key_disable_rule.rs @@ -40,10 +40,12 @@ pub struct LastAuthenticationKeyError { /// keeps a valid authentication key at the security level of every authentication key it /// disables. /// -/// A valid key is an enabled authentication key that has not expired, whatever its key type: -/// `ECDSA_HASH160` keys, which identities created by older Dash Evo Tool versions carry, count. -/// The keys the identity keeps count, and so do the keys the same update adds. A key stands in -/// for the disabled one only where it can sign everything the disabled one could: +/// A valid key is an enabled authentication key that has not expired and whose key type can sign +/// state transitions ([`KeyType::can_sign_state_transitions`](crate::identity::KeyType::can_sign_state_transitions)): `ECDSA_HASH160` keys, which +/// identities created by older Dash Evo Tool versions carry, count; `BIP13_SCRIPT_HASH` and +/// `EDDSA_25519_HASH160` keys do not. The keys the identity keeps count, and so do the keys the +/// same update adds. A key stands in for the disabled one only where it can sign everything the +/// disabled one could: /// /// - **Budget.** A key with a `total_budget` never counts: what is left of the budget lives in /// Drive, and an exhausted key can no longer sign. @@ -72,6 +74,7 @@ pub fn check_keys_can_be_disabled( .chain(add_public_keys) .filter(|key| { key.purpose() == Purpose::AUTHENTICATION + && key.key_type().can_sign_state_transitions() && key.disabled_at().is_none() && key.total_budget().is_none() && is_unexpired(key, now_ms) @@ -231,6 +234,38 @@ pub(crate) mod tests { assert!(check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)).is_err()); } + #[test] + fn should_not_count_a_key_type_that_cannot_sign_as_remaining() { + for key_type in [KeyType::BIP13_SCRIPT_HASH, KeyType::EDDSA_25519_HASH160] { + let unsignable = key( + 4, + Purpose::AUTHENTICATION, + SecurityLevel::CRITICAL, + key_type, + ); + let refused = Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }); + + let held = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + unsignable.clone(), + ]); + assert_eq!( + check_keys_can_be_disabled(&held, &[], &[1], Some(NOW_MS)), + refused, + "a held {key_type:?} key must not stand in" + ); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[unsignable], &[1], Some(NOW_MS)), + refused, + "an added {key_type:?} key must not stand in" + ); + } + } + #[test] fn should_allow_disabling_a_key_replaced_in_the_same_update() { let replacement = authentication_key(4, SecurityLevel::CRITICAL); diff --git a/packages/wasm-sdk/src/state_transitions/identity.rs b/packages/wasm-sdk/src/state_transitions/identity.rs index 988f305bd11..b00d87bfa8e 100644 --- a/packages/wasm-sdk/src/state_transitions/identity.rs +++ b/packages/wasm-sdk/src/state_transitions/identity.rs @@ -537,7 +537,12 @@ export interface IdentityUpdateOptions { /** * Array of key IDs to disable. - * Cannot disable master, critical auth, or transfer keys. + * The update is refused before signing if it would disable the last valid authentication + * key of a security level (DIP-11). A valid key is enabled, has no expiry and no budget, and + * has a key type that can sign (not BIP13_SCRIPT_HASH or EDDSA_25519_HASH160). It stands in + * at its own level or, unless it is MASTER, at any weaker level, and only for keys with no + * contract bounds or the same bounds as its own. Keys added in the same update count. Keys of + * other purposes are not restricted. */ disablePublicKeys?: number[];