diff --git a/packages/rs-platform-wallet/src/wallet/core/mod.rs b/packages/rs-platform-wallet/src/wallet/core/mod.rs index dbc42285aa2..36fe92318d6 100644 --- a/packages/rs-platform-wallet/src/wallet/core/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/core/mod.rs @@ -10,5 +10,6 @@ pub mod wallet; pub use balance::WalletBalance; pub use balance_handler::BalanceUpdateHandler; pub use generation::WalletGeneration; +pub(crate) use transaction::resolve_source_accounts; pub use transaction::{SignedCoreTransaction, ASSET_LOCK_FUNDING_SOURCES, SEND_FUNDING_SOURCES}; pub use wallet::CoreWallet; diff --git a/packages/rs-platform-wallet/src/wallet/core/transaction.rs b/packages/rs-platform-wallet/src/wallet/core/transaction.rs index 3ee44e049f6..d7f883cf1e0 100644 --- a/packages/rs-platform-wallet/src/wallet/core/transaction.rs +++ b/packages/rs-platform-wallet/src/wallet/core/transaction.rs @@ -266,7 +266,7 @@ pub const ASSET_LOCK_FUNDING_SOURCES: [AccountTypePreference; 3] = SEND_FUNDING_ /// DashPay source. A set selector matching nothing resolves to an empty list, /// not an error — a wallet with no contacts still sends from its standard /// accounts. -fn resolve_source_accounts( +pub(crate) fn resolve_source_accounts( accounts: &key_wallet::account::ManagedAccountCollection, preference: AccountTypePreference, source_index: u32, 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 4a01e667424..0110458e026 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -71,6 +71,14 @@ impl ContactRequestValidation { self.hard_error = true; } + /// Add an ABSENT-KEY error: the referenced key id does not exist on the + /// identity *today*. Sets `is_valid = false` but NOT `hard_error`, because + /// identities gain keys — see [`is_permanent`](Self::is_permanent). + pub fn add_absent_key_error(&mut self, error: String) { + self.errors.push(error); + 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. @@ -89,11 +97,31 @@ 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 / - /// missing / wrong-type key) is NOT purpose-only and must stay permanent. + /// 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. + /// + /// The distinction is not "did validation fail" but "can the world change + /// such that it stops failing". A `contactRequest` clears consensus without + /// consensus checking anything about the keys it names, so a document can + /// reference a key id our identity does not have *yet*: identities gain + /// keys (that is what the DashPay enablement flow does, and what + /// dashwallet-ios#981 exists to notice when it happened on another device). + /// Recording that as permanent turns a temporary gap into a relationship + /// 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 + /// permanent is what immutable facts make impossible: a key whose *type* + /// cannot do ECDH, and a key we have deliberately disabled. + pub fn is_permanent(&self) -> bool { + self.hard_error + } + /// Merge another validation result into this one. pub fn merge(&mut self, other: ContactRequestValidation) { self.errors.extend(other.errors); @@ -165,12 +193,28 @@ pub fn validate_contact_request( sender_key_index: u32, recipient_identity: &Identity, recipient_key_index: u32, +) -> ContactRequestValidation { + let mut validation = validate_sender_key(sender_identity, sender_key_index); + validation.merge(validate_recipient_key( + recipient_identity, + recipient_key_index, + )); + validation +} + +/// The sender half of [`validate_contact_request`] — the checks that need the +/// **counterparty's** identity. +/// +/// Crate-private: external callers go through the complete +/// [`validate_contact_request`] contract. Split out so the deferred-crypto +/// drain can run the recipient half first — see [`validate_recipient_key`] for +/// why that ordering matters, and what it changes for a mixed failure. +pub(crate) fn validate_sender_key( + sender_identity: &Identity, + sender_key_index: u32, ) -> ContactRequestValidation { let mut validation = ContactRequestValidation::new(); - // ----------------------------------------------------------------------- - // Sender key validation - // ----------------------------------------------------------------------- match sender_identity.get_public_key_by_id(sender_key_index) { Some(key) => { // Must be ECDSA_SECP256K1 for ECDH. @@ -205,7 +249,7 @@ pub fn validate_contact_request( } } None => { - validation.add_error(format!( + validation.add_absent_key_error(format!( "Sender key index {} not found on identity {}", sender_key_index, sender_identity.id(), @@ -213,9 +257,44 @@ pub fn validate_contact_request( } } - // ----------------------------------------------------------------------- - // Recipient key validation - // ----------------------------------------------------------------------- + validation +} + +/// The recipient half of [`validate_contact_request`] — the checks that need +/// only **our own** identity, which is always already resident. +/// +/// 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 +/// 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 +/// (missing / disabled / wrong type), 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. +/// +/// 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 +/// 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 +/// irreversible one. A sender-side hard fault still marks the channel broken +/// the moment our own key stops being the blocker. +pub(crate) fn validate_recipient_key( + recipient_identity: &Identity, + recipient_key_index: u32, +) -> ContactRequestValidation { + let mut validation = ContactRequestValidation::new(); + match recipient_identity.get_public_key_by_id(recipient_key_index) { Some(key) => { // Must be an ECDSA variant for ECDH compatibility. @@ -268,7 +347,7 @@ pub fn validate_contact_request( } } None => { - validation.add_error(format!( + validation.add_absent_key_error(format!( "Recipient key index {} not found on identity {}", recipient_key_index, recipient_identity.id(), @@ -576,6 +655,51 @@ mod tests { assert!(!result.purpose_mismatch); } + /// A key id the identity does not have **yet** must not be permanent. + /// + /// Identities gain keys — that is what the DashPay enablement flow does, + /// and dashwallet-ios#981 exists to notice it happening on another device. + /// A `contactRequest` clears consensus without consensus checking anything + /// about the keys it names, and it can never be re-minted, so recording + /// "we have no key 5 today" as a permanent verdict ends a relationship over + /// a gap that may close on its own — and only the CONTACT can clear the + /// flag, so the user cannot appeal it. + #[test] + fn an_absent_key_is_not_a_permanent_fault() { + let sender = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + Purpose::ENCRYPTION, + )]); + let recipient = make_identity(vec![]); + + let result = validate_contact_request(&sender, 0, &recipient, 0); + assert!(!result.is_valid, "an absent key still fails validation"); + assert!( + !result.is_permanent(), + "but it must be retryable: the identity can gain the key later" + ); + } + + /// A key type that cannot do ECDH is permanent — a key's type is fixed for + /// its lifetime, so no future state makes this request usable. + #[test] + fn a_non_ecdh_key_type_is_a_permanent_fault() { + let sender = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + Purpose::ENCRYPTION, + )]); + let recipient = make_identity(vec![make_key(0, KeyType::BLS12_381, Purpose::ENCRYPTION)]); + + let result = validate_contact_request(&sender, 0, &recipient, 0); + assert!(!result.is_valid); + assert!( + result.is_permanent(), + "a BLS key can never do secp256k1 ECDH, so this one may break the channel" + ); + } + /// 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 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 b197cc01e7b..c99018792cf 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 @@ -1940,7 +1940,12 @@ impl DashPayView<'_, B> { // reference those ids and nothing downstream makes sense without // 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 + // that work unconditional would add recurring cost to the very path + // this change exists to make cheap. + if tracing::enabled!(tracing::Level::INFO) { use dpp::identity::accessors::IdentityGettersV0; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; let owners: std::collections::BTreeSet = entries @@ -1984,6 +1989,15 @@ 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 = + 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 // entry's worth can ever be in flight. @@ -2099,6 +2113,74 @@ 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. + // + // Deciding here means a MIXED failure — our key + // purpose-rejected and the contact's key hard-faulted — + // now leaves the entry queued where the composed validator + // would have marked the channel broken. Deliberate: see + // `validate_recipient_key`. Marking broken is unappealable + // by the user, and the retry it avoids no longer costs a + // fetch. + let our_identity = { + let wm = self.wallet_manager.read().await; + wm.get_wallet_info(&self.wallet_id) + .and_then(|info| { + info.identity_manager + .managed_identity(&entry.owner_identity_id) + }) + .map(|m| m.identity.clone()) + }; + let Some(our_identity) = our_identity else { + tracing::warn!( + owner = %entry.owner_identity_id, contact = %entry.contact_id, + "drain: our identity vanished mid-drain; leaving queued" + ); + 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 + // 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 + // a decrypt failure from being charged to the document. + // + // The recipient term is known here; the sender term needs + // the identity fetched below and is OR-ed in there. + let recipient_widened = our_identity + .get_public_key_by_id(*our_decryption_key_index) + .map(|k| { + !dash_sdk::platform::dashpay::recipient_key_purpose_is_valid( + k.purpose(), + ) + }) + .unwrap_or(false); + let recipient_validation = + crate::wallet::identity::crypto::validation::validate_recipient_key( + &our_identity, + *our_decryption_key_index, + ); + if !recipient_validation.is_valid { + if self + .apply_drain_validation_failure( + entry, + recipient_validation, + &mut policy_blocked, + ) + .await + { + cleared.push(entry.key()); + } + continue; + } + // Fetch the contact identity (transient on failure → leave). // Bounded: a Platform round trip, and nothing in this entry // has committed yet. @@ -2132,88 +2214,31 @@ impl DashPayView<'_, B> { } }; - // Validate key indices (purpose + type) BEFORE ECDH — the - // same gate the resident sweep path applies, so the deferred - // path enforces the identical contract. A purpose-only - // mismatch (e.g. a legacy doc referencing an AUTH key) is left - // queued for a future acceptance-policy change; a hard failure - // (key type / missing / disabled) marks the channel broken and - // clears the entry. - let our_identity = { - let wm = self.wallet_manager.read().await; - wm.get_wallet_info(&self.wallet_id) - .and_then(|info| { - info.identity_manager - .managed_identity(&entry.owner_identity_id) - }) - .map(|m| m.identity.clone()) - }; - let Some(our_identity) = our_identity else { - tracing::warn!( - owner = %entry.owner_identity_id, contact = %entry.contact_id, - "drain: our identity vanished mid-drain; leaving queued" - ); - 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 legacy dashj cohort, whose ECDH/AES byte - // compatibility with our implementation has not been - // cross-validated against a dashj-produced payload. Used - // below to keep a decrypt failure from being charged to the - // document. - // - // BOTH sides matter: the widening moved the sender policy - // from ENCRYPTION-only to ENCRYPTION-or-AUTHENTICATION too, - // so an AUTHENTICATION sender paired with a mint-valid - // recipient is just as much an unverified legacy payload as - // the recipient-side case, and equally must not have a - // convention gap charged to it. - let accepted_by_legacy_widening = { - let recipient_widened = our_identity - .get_public_key_by_id(*our_decryption_key_index) - .map(|k| { - !dash_sdk::platform::dashpay::recipient_key_purpose_is_valid( - k.purpose(), - ) - }) - .unwrap_or(false); - // The mint-side sender rule is ENCRYPTION-only; anything - // else reaching here was admitted by the widening. - let sender_widened = contact_identity + // 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. + let accepted_by_legacy_widening = recipient_widened + || contact_identity .get_public_key_by_id(*contact_encryption_key_index) .map(|k| k.purpose() != Purpose::ENCRYPTION) .unwrap_or(false); - recipient_widened || sender_widened - }; + + // The sender half — the checks that need the identity we + // just fetched. let validation = - crate::wallet::identity::crypto::validation::validate_contact_request( + crate::wallet::identity::crypto::validation::validate_sender_key( &contact_identity, *contact_encryption_key_index, - &our_identity, - *our_decryption_key_index, ); if !validation.is_valid { - if validation.is_purpose_only() { - tracing::warn!( - owner = %entry.owner_identity_id, contact = %entry.contact_id, - errors = ?validation.errors, - "drain: contact request key-purpose mismatch; leaving queued (not marking broken)" - ); - continue; + if self + .apply_drain_validation_failure(entry, validation, &mut policy_blocked) + .await + { + cleared.push(entry.key()); } - tracing::warn!( - owner = %entry.owner_identity_id, contact = %entry.contact_id, - errors = ?validation.errors, - "drain: contact request failed key-index validation; marking channel broken" - ); - self.mark_contact_channel_broken( - &entry.owner_identity_id, - &entry.contact_id, - ) - .await; - cleared.push(entry.key()); continue; } @@ -2243,16 +2268,17 @@ impl DashPayView<'_, B> { } } None => { + // Left queued, not broken: the contact's identity + // can gain this key later, exactly as ours can, and + // the document that names it cleared consensus and + // cannot be re-minted. Breaking here would end the + // relationship over a gap that may close by itself. tracing::warn!( owner = %entry.owner_identity_id, contact = %entry.contact_id, - "drain: contact encryption key missing; marking channel broken" + key_index = *contact_encryption_key_index, + "drain: contact has no key at the referenced index yet; \ + leaving queued (not marking broken)" ); - self.mark_contact_channel_broken( - &entry.owner_identity_id, - &entry.contact_id, - ) - .await; - cleared.push(entry.key()); continue; } }; @@ -2443,6 +2469,21 @@ 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 + // 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(); + 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)" + ); + } // One-line verdict for the pass. "Did the legacy contacts build?" is // answerable from this alone, without counting per-entry lines across a // multi-megabyte export. @@ -2456,6 +2497,53 @@ impl DashPayView<'_, B> { drained_total } + /// 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` + /// 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. + /// - 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. + /// + /// Returns `true` when the caller should clear the entry from the queue. + /// + /// Takes `validation` by value: a purpose-rejected 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 + /// the result afterwards. + async fn apply_drain_validation_failure( + &self, + entry: &crate::changeset::PendingContactCrypto, + validation: crate::wallet::identity::crypto::validation::ContactRequestValidation, + policy_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)" + ); + for reason in validation.errors { + *policy_blocked.entry(reason).or_default() += 1; + } + return false; + } + tracing::warn!( + owner = %entry.owner_identity_id, contact = %entry.contact_id, + errors = ?validation.errors, + "drain: contact request failed key-index validation; marking channel broken" + ); + self.mark_contact_channel_broken(&entry.owner_identity_id, &entry.contact_id) + .await; + true + } + /// Apply the dequeue for entries a drain just completed: remove them from /// their owners' in-memory queues and persist the removal. Returns how many /// were actually removed. diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs index 7d2ef339a96..821bfe9e4d9 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs @@ -450,22 +450,24 @@ impl DashPayView<'_, B> { // --- 2. Decrypt the contact's xpub with the signer-derived secret. --- // - // This failing is the single most diagnostic event on the whole path, - // so the message carries what tells the two hypotheses apart. AES-CBC - // with a WRONG key yields pseudorandom bytes, and PKCS7 then rejects - // them ~99.6% of the time — so a failure here means the ECDH shared - // secret did not match the sender's, i.e. a key-derivation or - // ECDH-convention gap, NOT a corrupt document. (A failure at step 3 - // below means the opposite: the secret was right and the plaintext - // layout is what differs.) The ciphertext length is included because a - // non-96-byte blob would instead point at a malformed document, which - // the contract's minItems/maxItems: 96 should already have prevented. + // This failing is the most diagnostic event on the whole path, so the + // message names what it points at — as a likelihood, not a verdict. + // AES-CBC is unauthenticated here and PKCS7 is the only check on the + // plaintext, so a padding rejection is consistent with a mismatched + // ECDH secret AND with a corrupted or malformed ciphertext; a wrong key + // also clears padding roughly 1 in 256 times and lands at step 3 + // instead. Neither outcome proves which, and stating otherwise would + // misdirect exactly the legacy-interop investigation these messages + // exist to serve. The ciphertext length is included because a + // non-96-byte blob points at a malformed document, which the contract's + // minItems/maxItems: 96 should already have prevented. let decrypted_xpub_bytes = platform_encryption::decrypt_extended_public_key(&shared_key, contact_encrypted_xpub) .map_err(|e| { Permanent(PlatformWalletError::InvalidIdentityData(format!( - "Failed to decrypt contact xpub ({e}); ciphertext {} bytes — the ECDH \ - shared secret did not match the sender's (PKCS7 rejected the plaintext)", + "Failed to decrypt contact xpub ({e}); ciphertext {} bytes. PKCS7 rejected \ + the plaintext — most likely the ECDH shared secret did not match the \ + sender's, though a corrupted ciphertext produces the same symptom", contact_encrypted_xpub.len() ))) })?; @@ -491,17 +493,19 @@ impl DashPayView<'_, B> { .map_err(Permanent)?, Err(_) => { key_wallet::bip32::ExtendedPubKey::decode(&decrypted_xpub_bytes).map_err(|e| { - // Reaching here means the DECRYPT succeeded — PKCS7 unpadded - // cleanly, so the shared secret was almost certainly right — - // and only the plaintext LAYOUT is unexpected. The decrypted - // length is the discriminator, so it leads the message. The - // bytes themselves are never logged: they are the contact's + // PKCS7 unpadded cleanly but the plaintext is not a shape we + // know. That is consistent with a correct secret over an + // unexpected LAYOUT, and also with a wrong key whose garbage + // happened to carry valid padding (~1 in 256) — the length + // is the best discriminator available, so it leads the + // message, but it is not proof either way. The bytes + // themselves are never logged: they are the contact's // payment xpub, and this text reaches an exported log. Permanent(PlatformWalletError::InvalidIdentityData(format!( "Decrypted contact xpub is {} bytes — neither a 69-byte DIP-15 compact \ - form nor a 78/107-byte BIP32/DIP-14 serialization ({e}). The decrypt \ - itself SUCCEEDED, so the shared secret matched and it is the plaintext \ - layout that differs", + form nor a 78/107-byte BIP32/DIP-14 serialization ({e}). PKCS7 accepted \ + the plaintext, which suggests the shared secret matched and the layout \ + differs, but unauthenticated CBC also lets a wrong key land here", decrypted_xpub_bytes.len() ))) })? 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 88505879b2d..c084ea667a1 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -1098,7 +1098,7 @@ impl DashPayView<'_, B> { // re-acquires that (non-reentrant) lock internally. self.drain_pending_contact_crypto(provider).await; - let (payment_address, used_flip_changeset, tx, fee) = { + let (payment_address, used_flip_changeset, tx, fee, funding_accounts) = { let mut wm = self.wallet_manager.write().await; // Resolve the external account's xpub so we can derive addresses. @@ -1199,32 +1199,72 @@ impl DashPayView<'_, B> { let current_height = info.core_wallet.synced_height(); - let managed_account = info - .core_wallet - .accounts - .standard_bip44_accounts - .get_mut(&0) - .ok_or_else(|| { - PlatformWalletError::TransactionBuild( - "BIP-44 managed account 0 not found".to_string(), - ) - })?; - let account = wallet - .accounts - .standard_bip44_accounts - .get(&0) - .ok_or_else(|| { - PlatformWalletError::TransactionBuild( - "BIP-44 account 0 not found in wallet".to_string(), - ) - })?; - - let builder = TransactionBuilder::new() + // Pool the same funding set as a plain send (#4329): BIP44 + + // BIP32 + every DashPay receiving account. Pinning this path to + // BIP44 alone was the reason a wallet whose balance had moved into + // contact-receiving accounts hit "Insufficient funds" on a screen + // showing plenty — the exact symptom #4329 fixed for the core send + // path, which this path never picked up (it only took that PR's + // `set_funding` → `add_funding` rename). + // + // Order is load-bearing: BIP44 is offered first, and the builder + // takes the change address from the first funding source, so + // change keeps returning to BIP44 as before. CoinJoin stays out by + // construction — spending mixed outputs alongside transparent ones + // links them and undoes the mixing — and so do the contact + // *external* accounts, which hold the counterparty's xpub and no + // key this wallet can sign with. + let mut builder = TransactionBuilder::new() .set_current_height(current_height) .set_selection_strategy(SelectionStrategy::LargestFirst) - .add_funding(managed_account, account) .add_output(&payment_address, amount_duffs); + // Derivation paths for every offered UTXO, since the signer closure + // below can no longer resolve them from one account. + let mut funding_paths: std::collections::HashMap< + dashcore::Address, + key_wallet::bip32::DerivationPath, + > = std::collections::HashMap::new(); + // Accounts whose UTXOs were OFFERED to selection. A superset of the + // contributors — releasing a reservation on an account that + // supplied nothing is a no-op, and the superset is what keeps the + // rejection path from stranding inputs in an account we forgot. + let mut offered_accounts: Vec = Vec::new(); + + for &preference in crate::SEND_FUNDING_SOURCES.iter() { + for at in crate::wallet::core::resolve_source_accounts( + &info.core_wallet.accounts, + preference, + account_index, + ) { + if offered_accounts.contains(&at) { + continue; + } + // A source the wallet simply does not have contributes + // nothing rather than failing the send — a wallet with no + // BIP32 account, or no contacts, still pays from BIP44. + let (Some(account), Some(managed)) = ( + wallet.accounts.account_of_type(at), + info.core_wallet.accounts.funds_account_mut(&at), + ) else { + continue; + }; + for utxo in managed.utxos.values() { + if let Some(path) = managed.address_derivation_path(&utxo.address) { + funding_paths.insert(utxo.address.clone(), path); + } + } + builder = builder.add_funding(managed, account); + offered_accounts.push(at); + } + } + if offered_accounts.is_empty() { + return Err(PlatformWalletError::TransactionBuild( + "no spendable funding account (BIP44/BIP32/DashPay receiving) found" + .to_string(), + )); + } + // Sign through the injected signer (blanket // `impl TransactionSigner for S`) rather than the // resident `wallet`, so funding-input signatures are produced @@ -1235,9 +1275,7 @@ impl DashPayView<'_, B> { // rust-dashcore#872 (pinned above). No caller-side // recomputation needed. let (tx, fee) = match builder - .build_signed(signer, |addr| { - managed_account.address_derivation_path(&addr) - }) + .build_signed(signer, |addr| funding_paths.get(&addr).cloned()) .await { Ok(built) => built, @@ -1269,7 +1307,13 @@ impl DashPayView<'_, B> { } }; - (payment_address, used_flip_changeset, tx, fee) + ( + payment_address, + used_flip_changeset, + tx, + fee, + offered_accounts, + ) }; // Persist the payment-address used flip now that the wallet-manager @@ -1291,16 +1335,28 @@ impl DashPayView<'_, B> { // --- 3. Broadcast the transaction, releasing the build's UTXO // reservation if the broadcast is definitively rejected pre-send. --- - let txid = match crate::wallet::reservations::broadcast_releasing_on_rejection( - self.broadcaster.as_ref(), - &self.wallet_manager, - &self.wallet_id, - key_wallet::account::account_type::StandardAccountType::BIP44Account, - 0, - &tx, - ) - .await - { + // Release across EVERY account that offered inputs, not just BIP44: + // now that the build pools funding, a rejected broadcast whose inputs + // came from a BIP32 or contact-receiving account would otherwise leave + // those reserved until the TTL backstop, and an immediate retry would + // fail with a spurious insufficient-funds. + let broadcast_result = match self.broadcaster.broadcast(&tx).await { + Err(e) if matches!(e, crate::broadcaster::BroadcastError::Rejected { .. }) => { + crate::wallet::reservations::release_reservation_after_rejected_broadcast( + &self.wallet_manager, + &self.wallet_id, + &funding_accounts, + &tx, + // This path does not thread the build's reservation token + // either; keep the historical unconditional release. + None, + ) + .await; + Err(e) + } + other => other, + }; + let txid = match broadcast_result { Ok(txid) => txid, Err(e) => { // A definitive rejection means the transaction never reached @@ -1795,6 +1851,44 @@ mod tests { .xpub } + /// An identity carrying exactly one key, for the validation paths that + /// turn on a key's type or purpose rather than its presence. + fn identity_with_key( + id_bytes: [u8; 32], + key_id: u32, + key_type: dpp::identity::KeyType, + purpose: dpp::identity::Purpose, + ) -> Identity { + use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dpp::identity::{IdentityPublicKey, SecurityLevel}; + let data = dashcore::secp256k1::PublicKey::from_secret_key( + &dashcore::secp256k1::Secp256k1::new(), + &dashcore::secp256k1::SecretKey::from_slice(&[0x37u8; 32]).expect("secret"), + ) + .serialize() + .to_vec(); + Identity::V0(IdentityV0 { + id: Identifier::from(id_bytes), + public_keys: [( + key_id, + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: key_id, + purpose, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type, + read_only: false, + data: data.into(), + disabled_at: None, + }), + )] + .into_iter() + .collect(), + balance: 0, + revision: 0, + }) + } + fn bare_identity(id_bytes: [u8; 32]) -> Identity { Identity::V0(IdentityV0 { id: Identifier::from(id_bytes), @@ -5026,6 +5120,257 @@ 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. + /// + /// This is the discriminating test for both halves of the change: + /// + /// * The contact identity IS configured on the mock, and its key at the + /// sender index is missing — a hard fault. Under the old composed + /// validation the drain would fetch, merge both halves, see `hard_error`, + /// 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. + /// + /// Drained twice, because the cost this PR removes is per sweep, not once. + #[tokio::test] + async fn unaccepted_recipient_purpose_never_fetches_and_stays_recoverable() { + use crate::changeset::{PendingContactCrypto, PendingContactCryptoOp}; + use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; + use crate::wallet::identity::{ContactRequest, EstablishedContact}; + use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dpp::identity::{IdentityPublicKey, IdentityV0, KeyType, Purpose, SecurityLevel}; + + 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. + let our_key = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: 0, + purpose: Purpose::VOTING, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: dashcore::secp256k1::PublicKey::from_secret_key( + &dashcore::secp256k1::Secp256k1::new(), + &dashcore::secp256k1::SecretKey::from_slice(&[0x24u8; 32]).expect("secret"), + ) + .serialize() + .to_vec() + .into(), + disabled_at: None, + }); + let our_identity = Identity::V0(IdentityV0 { + id: owner, + public_keys: [(0u32, our_key)].into_iter().collect(), + balance: 0, + revision: 0, + }); + + // The contact identity the drain WOULD fetch: its key at the sender + // index is BLS, a permanent fault. Configured on the mock so that a + // fetch, if it happened, would succeed and escalate the verdict to + // "broken". (A keyless contact would not work as the discriminator — + // an absent key is retryable by design.) + let mut sdk = dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"); + sdk.mock() + .expect_fetch::( + contact, + Some(identity_with_key( + [0xBB; 32], + 0, + KeyType::BLS12_381, + Purpose::ENCRYPTION, + )), + ) + .await + .expect("set the contact-identity fetch expectation"); + let sdk = Arc::new(sdk); + + let persister = Arc::new(RecordingPersister::default()); + let handler: Arc = Arc::new(NoopEventHandler); + let manager = Arc::new(PlatformWalletManager::new( + sdk, + Arc::clone(&persister), + handler, + )); + let seed = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid mnemonic") + .to_seed(""); + let wallet_id = manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("wallet creation") + .wallet_id(); + let wallet_arc = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet_arc.identity(); + let p = WalletPersister::new(wallet_id, Arc::clone(&persister) as _); + + { + let mut wm = iw.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + info.identity_manager + .add_identity(our_identity, 0, wallet_id, &p) + .expect("add owner"); + let outgoing = ContactRequest::new(owner, contact, 0, 0, 0, vec![0u8; 96], 0, 0); + let incoming = ContactRequest::new(contact, owner, 0, 0, 0, vec![0u8; 96], 0, 0); + let managed = info + .identity_manager + .managed_identity_mut(&owner) + .expect("owner resident"); + managed.apply_established_contact(EstablishedContact::new(contact, outgoing, incoming)); + managed + .dashpay_pending_contact_crypto_mut() + .push(PendingContactCrypto { + owner_identity_id: owner, + contact_id: contact, + op: PendingContactCryptoOp::RegisterExternal { + encrypted_public_key: vec![7u8; 96], + our_decryption_key_index: 0, + contact_encryption_key_index: 0, + }, + enqueued_at_ms: 0, + }); + } + + let provider = SeedCryptoProvider::from_seed(seed, Network::Testnet); + for pass in 1..=2 { + let drained = iw.dashpay().drain_pending_contact_crypto(&provider).await; + assert_eq!( + drained, 0, + "pass {pass}: a purpose-rejected entry must stay queued, not be cleared" + ); + } + + let wm = iw.wallet_manager.read().await; + let managed = wm + .get_wallet_info(&wallet_id) + .expect("info") + .identity_manager + .managed_identity(&owner) + .expect("owner resident"); + assert_eq!( + managed.dashpay().pending_contact_crypto.len(), + 1, + "the entry must survive repeated drains so a policy change can still pick it up" + ); + assert!( + !managed + .dashpay() + .established_contacts() + .get(&contact) + .expect("contact resident") + .payment_channel_broken, + "the channel must stay intact — reaching this verdict without the configured \ + fetch being consumed is what proves no Platform round trip was spent" + ); + } + + /// A `RegisterExternal` entry whose fault lies in OUR OWN key is decided + /// without a Platform round trip. + /// + /// The owner here is wallet-owned (so the drain gets past the HD-index + /// bail) and its key at `recipientKeyIndex` 0 is BLS — a type that can + /// never do ECDH, so this is one of the few genuinely permanent faults and + /// must break the channel. (An *absent* key would not do: identities gain + /// keys, so that is deliberately retryable.) The mock SDK has NO + /// contact-identity fetch configured, so this can only pass if the + /// recipient half of the validation ran *before* the fetch: the old + /// ordering fetched first, failed transiently, and left the channel + /// intact. + /// + /// That ordering is what keeps a purpose-rejected entry — which stays + /// queued by design, and so is retried on every sweep forever — from + /// spending a network round trip each time. + #[tokio::test] + async fn drain_decides_our_own_key_fault_without_fetching_the_contact() { + use crate::changeset::{PendingContactCrypto, PendingContactCryptoOp}; + use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; + use crate::wallet::identity::{ContactRequest, EstablishedContact}; + use dpp::identity::{KeyType, Purpose}; + + let (manager, persister, wallet_id) = make_wallet().await; + let wallet_arc = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet_arc.identity(); + let owner = Identifier::from([0xAA; 32]); + let contact = Identifier::from([0xBB; 32]); + let p = WalletPersister::new(wallet_id, Arc::clone(&persister) as _); + + { + let mut wm = iw.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + // Wallet-owned (HD index 0) with a BLS key at index 0: the + // referenced key exists but its type rules out ECDH permanently. + info.identity_manager + .add_identity( + identity_with_key([0xAA; 32], 0, KeyType::BLS12_381, Purpose::ENCRYPTION), + 0, + wallet_id, + &p, + ) + .expect("add owner"); + let outgoing = ContactRequest::new(owner, contact, 0, 0, 0, vec![0u8; 96], 0, 0); + let incoming = ContactRequest::new(contact, owner, 0, 0, 0, vec![0u8; 96], 0, 0); + let managed = info + .identity_manager + .managed_identity_mut(&owner) + .expect("owner resident"); + managed.apply_established_contact(EstablishedContact::new(contact, outgoing, incoming)); + managed + .dashpay_pending_contact_crypto_mut() + .push(PendingContactCrypto { + owner_identity_id: owner, + contact_id: contact, + op: PendingContactCryptoOp::RegisterExternal { + encrypted_public_key: vec![7u8; 96], + our_decryption_key_index: 0, + contact_encryption_key_index: 0, + }, + enqueued_at_ms: 0, + }); + } + + let provider = SeedCryptoProvider::from_seed( + Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid mnemonic") + .to_seed(""), + Network::Testnet, + ); + let drained = iw.dashpay().drain_pending_contact_crypto(&provider).await; + + assert_eq!( + drained, 1, + "a hard validation fault must clear the entry rather than retry it forever" + ); + let wm = iw.wallet_manager.read().await; + let managed = wm + .get_wallet_info(&wallet_id) + .expect("info") + .identity_manager + .managed_identity(&owner) + .expect("owner resident"); + assert!( + managed + .dashpay() + .established_contacts() + .get(&contact) + .expect("contact resident") + .payment_channel_broken, + "the channel must be marked broken from our own key alone — reaching this \ + verdict proves the recipient half ran before the (unconfigured) contact fetch" + ); + } + /// The whole external-account build works with a **legacy key id and /// purpose** — derivation at that id, ECDH, AES decrypt, compact-xpub /// parse, registration — not just the purpose predicate. @@ -5717,6 +6062,110 @@ mod tests { } } + /// A contact payment funds from a DashPay **receiving** account when BIP44 + /// alone cannot cover it — the pooled funding set a plain send has used + /// since #4329. + /// + /// This path kept its BIP44-only pin through that PR (it took only the + /// `set_funding` → `add_funding` rename), so a wallet whose balance had + /// moved into contact-receiving accounts saw the funds in its total and got + /// `Insufficient funds` trying to pay a contact. Reported from mainnet + /// after 8 successful contact payments drained BIP44: `available 41505, + /// required 100000`, on a screen showing plenty. + /// + /// BIP44 is left empty here, so reaching the signer at all proves the + /// receiving account was offered to selection. + #[tokio::test] + async fn contact_payment_funds_from_a_dashpay_receiving_account() { + use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; + + let (manager, persister, wallet_id) = make_wallet().await; + let wallet_arc = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet_arc.identity(); + + let owner_id = Identifier::from([0x11; 32]); + let contact_id = Identifier::from([0x22; 32]); + { + let mut wm = iw.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + info.identity_manager + .add_identity( + bare_identity([0x11; 32]), + 0, + wallet_id, + &WalletPersister::new(wallet_id, Arc::clone(&persister) as _), + ) + .expect("add owner"); + } + + // The receiving side: register the account, then give it the wallet's + // only money. BIP44 stays empty. + iw.dashpay() + .register_contact_account( + &owner_id, + &contact_id, + 0, + test_receiving_xpub(&owner_id, &contact_id), + ) + .await + .expect("register receiving account"); + plant_receival_utxo(&manager, wallet_id, owner_id, contact_id, 0x21, 1_000_000).await; + + // The sending side, so the external-account lookup passes. + let shared_key = [0x55u8; 32]; + let seed = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("mnemonic") + .to_seed(""); + let compact = { + let w = key_wallet::wallet::Wallet::from_seed_bytes( + seed, + Network::Testnet, + WalletAccountCreationOptions::None, + ) + .expect("seed wallet"); + crate::wallet::identity::crypto::dip14::derive_contact_xpub( + &w, + Network::Testnet, + 0, + &owner_id, + &contact_id, + ) + .expect("derive a valid compact xpub") + .compact + .to_bytes() + }; + let encrypted = + platform_encryption::encrypt_extended_public_key(&shared_key, &[0x11u8; 16], &compact); + iw.dashpay() + .register_external_contact_account( + &owner_id, + &bare_identity([0x22; 32]), + &encrypted, + zeroize::Zeroizing::new(shared_key), + ) + .await + .expect("register external account"); + + let provider = SeedCryptoProvider::from_seed(seed, Network::Testnet); + let signer = SeedSigner::new(seed, Network::Testnet); + let result = iw + .dashpay() + .send_payment(&owner_id, &contact_id, 100_000, None, &signer, &provider) + .await; + + // Whatever happens later (this wallet has no live broadcaster), the one + // outcome the fix rules out is coin selection refusing for lack of + // funds while a funded receiving account sits right there. + if let Err(e) = &result { + let msg = e.to_string(); + assert!( + !msg.contains("Insufficient funds") && !msg.contains("No UTXOs available"), + "the contact-receiving account's 1_000_000 duffs must be offered to \ + selection — BIP44-only funding is the bug this pins, got: {msg}" + ); + } + } + /// A failed `build_signed` must return the consumed payment address to /// the pool. Without the rollback every failed build (insufficient /// funds, a refusing signer) permanently advances the next index by one: @@ -5827,6 +6276,72 @@ mod tests { ); } + /// A rejected broadcast releases the UTXO reservation on EVERY account + /// that funded the payment, not just BIP44. + /// + /// Pooling made this reachable: before it, one account funded the send and + /// releasing that one was complete. Now inputs can come from a BIP32 or + /// contact-receiving account too, and a release that still named only + /// BIP44 would leave those reserved until the TTL backstop — so the + /// immediate retry a user makes after "payment rejected" would fail with a + /// spurious insufficient-funds on money that is demonstrably theirs. + /// + /// Neither account can cover the payment alone here, so a successful retry + /// is only possible if BOTH were released. + #[tokio::test] + async fn rejected_broadcast_releases_every_pooled_funding_account() { + use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; + + let (manager, _persister, wallet_id, owner_id, contact_id) = + register_sender_and_external_account().await; + let wallet_arc = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet_arc.identity(); + + // 60_000 + 60_000, for a 100_000 payment: neither side alone is + // enough, so selection must take from both and a retry must find both + // free again. + fund_bip44_account_0(&manager, wallet_id, 0xC1, 60_000).await; + iw.dashpay() + .register_contact_account( + &owner_id, + &contact_id, + 0, + test_receiving_xpub(&owner_id, &contact_id), + ) + .await + .expect("register receiving account"); + plant_receival_utxo(&manager, wallet_id, owner_id, contact_id, 0xC2, 60_000).await; + + let seed = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid mnemonic") + .to_seed(""); + let provider = SeedCryptoProvider::from_seed(seed, Network::Testnet); + let signer = SeedSigner::new(seed, Network::Testnet); + + let rejecting = with_rejecting_broadcaster(iw); + let err = rejecting + .dashpay() + .send_payment(&owner_id, &contact_id, 100_000, None, &signer, &provider) + .await + .expect_err("the rejecting broadcaster must fail the send"); + assert!( + matches!(err, PlatformWalletError::TransactionBroadcast(_)), + "the send must reach the broadcast (so inputs were reserved), got: {err:?}" + ); + + // The retry is the assertion: it needs inputs from both accounts, so + // it can only succeed if the rejection released both reservations. + let accepting = with_accepting_broadcaster(iw); + accepting + .dashpay() + .send_payment(&owner_id, &contact_id, 100_000, None, &signer, &provider) + .await + .expect( + "an immediate retry must reselect every pooled input — a reservation left \ + on the contact-receiving account strands funds until the TTL backstop", + ); + } + /// A definitively rejected broadcast must return the consumed payment /// address to the pool AND persist the revert — unlike a failed build, /// the used flip was already persisted before the broadcast attempt, so