diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs b/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs index 900a5b07e7..a2f74633cf 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs @@ -254,6 +254,23 @@ impl WalletChangeSetFFI { /// (the records are authoritative), and re-deriving keeps the /// per-account routing self-contained. /// + /// That redundancy stopped holding for spends once + /// dashpay/platform#4363 began *suppressing* a contact's watch-only + /// record from `records` while still emitting its spend — the + /// stale-TXO heal. With nothing left in `records` to re-derive from, + /// those spends reached no mobile host at all; only the SQLite backend, + /// which reads `spent_utxos` directly, ever ran the heal. + /// `CoreChangeSet::unrecorded_spends` carries exactly that residue — + /// outpoint, spending txid and owning account — and is folded into the + /// per-account `utxos_spent` arrays below. + /// + /// That closes the gap *at this boundary only*. The entries now cross + /// the FFI, but neither host acts on them yet: both gate the `isSpent` + /// flip on resolving `spending_txid` to a persisted transaction row, and + /// for a suppressed record no such row is ever written. Do not read the + /// fold below as "the heal runs on mobile" — see + /// `CoreChangeSet::unrecorded_spends` for what still has to land. + /// /// `chain` carries `synced_height` from the changeset's /// `synced_height` field; `block_hash` is omitted because /// `WalletEvent::SyncHeightAdvanced` doesn't carry it (the upstream @@ -351,6 +368,17 @@ impl WalletChangeSetFFI { } } + // Same for an account carrying only suppressed spend-clears. This is + // the common shape of the #4363 heal: a contact spends a stale row + // and their watch-only record — the batch's only record for that + // account — is dropped, so the bucket would not exist at all and the + // spend would have nowhere to be emitted. + for spend in &cs.unrecorded_spends { + if !by_account.iter().any(|(at, _)| at == &spend.account_type) { + by_account.push((spend.account_type, Vec::new())); + } + } + let mut ffi_accounts = Vec::with_capacity(by_account.len()); for (account_type, recs) in by_account { let type_name = CString::new(format!("{:?}", account_type)) @@ -367,6 +395,16 @@ impl WalletChangeSetFFI { utxos_added.extend(record_new_utxos_ffi(rec)); utxos_spent.extend(record_spent_outpoints_ffi(rec)); } + // Fold in the spends whose record never made it into `records`. + // Disjoint from the loop above by construction — `unrecorded_spends` + // is populated only for records that are suppressed — so no entry + // is emitted twice. + utxos_spent.extend( + cs.unrecorded_spends + .iter() + .filter(|spend| spend.account_type == account_type) + .map(unrecorded_spend_ffi), + ); // Transactions for this account. let transactions: Vec = @@ -882,6 +920,36 @@ fn record_spent_outpoints_ffi( .collect() } +/// Project an [`UnrecordedSpend`] — a spend whose record was suppressed from +/// the changeset's `records` — into the same wire shape +/// [`record_spent_outpoints_ffi`] produces. +/// +/// The spending txid is carried through rather than zeroed: both host +/// handlers resolve it to decide whether the spend is final, and a zero txid +/// makes the emit a no-op on each of them. +/// +/// A non-zero txid is not sufficient either, and today it is not: the hosts +/// look the txid up in their own transaction table, and a suppressed record +/// never puts it there, so these entries currently land and do nothing. The +/// txid is carried because it is the one fact that cannot be recovered +/// downstream once the record is gone — see +/// `CoreChangeSet::unrecorded_spends`. +/// +/// [`UnrecordedSpend`]: platform_wallet::changeset::UnrecordedSpend +fn unrecorded_spend_ffi(spend: &platform_wallet::changeset::UnrecordedSpend) -> SpentOutPointFFI { + let mut txid = [0u8; 32]; + txid.copy_from_slice(spend.outpoint.txid.as_ref()); + let mut spending_txid = [0u8; 32]; + spending_txid.copy_from_slice(spend.spending_txid.as_ref()); + SpentOutPointFFI { + outpoint: OutPointFFI { + txid, + vout: spend.outpoint.vout, + }, + spending_txid, + } +} + /// Map upstream `TransactionType` to a stable `u8` discriminant for /// the FFI wire shape. Order mirrors the enum declaration in /// `key_wallet::transaction_checking::transaction_router::mod.rs`, @@ -1832,6 +1900,110 @@ mod tests { unsafe { free_wallet_changeset_ffi(&ffi) }; } + /// A contact spending a stale pre-#4363 TXO must cross the FFI as a + /// spend-clear, carrying the spending txid. + /// + /// This is the changeset shape the #4363 heal produces: the contact's + /// watch-only record is suppressed from `records` (it is not a + /// transaction of ours), so the batch has **no record at all** for that + /// account — and `from_changeset` built its buckets, and derived every + /// spend, from `records` alone. The clear reached the SQLite backend + /// (which reads `cs.spent_utxos` directly) and no mobile host. + /// + /// Pre-fix this emitted zero accounts and zero spends. + /// + /// Scope: this pins the *FFI boundary*, not the heal. Both host handlers + /// still require a persisted row for `spending_txid` before they touch + /// `isSpent`, and a suppressed record produces none — so what this test + /// asserts is that the entry arrives with everything a host would need, + /// not that any host acts on it today. + #[test] + fn contact_spend_of_a_stale_txo_crosses_the_ffi() { + use dashcore::hashes::Hash; + use platform_wallet::changeset::UnrecordedSpend; + + let account = AccountType::DashpayExternalAccount { + index: 0, + user_identity_id: [1u8; 32], + friend_identity_id: [2u8; 32], + }; + let stale_txid = dashcore::Txid::from_slice(&[0x11; 32]).expect("txid"); + let spending_txid = dashcore::Txid::from_slice(&[0x22; 32]).expect("txid"); + + let mut cs = CoreChangeSet::default(); + cs.unrecorded_spends.push(UnrecordedSpend { + outpoint: dashcore::OutPoint { + txid: stale_txid, + vout: 1, + }, + spending_txid, + account_type: account, + }); + + let ffi = WalletChangeSetFFI::from_changeset(&cs); + assert_eq!( + ffi.accounts_count, 1, + "the suppressed record's account must still get a bucket to carry \ + the spend-clear" + ); + let bucket = unsafe { &*ffi.accounts }; + assert_eq!( + bucket.transactions_count, 0, + "the contact's spend is still not a transaction row of ours" + ); + assert_eq!( + bucket.utxos_spent_count, 1, + "the stale TXO's spend-clear must cross the FFI" + ); + let spent = unsafe { &*bucket.utxos_spent }; + assert_eq!(spent.outpoint.txid, stale_txid.to_byte_array()); + assert_eq!(spent.outpoint.vout, 1); + assert_eq!( + spent.spending_txid, + spending_txid.to_byte_array(), + "the spending txid must survive — both host handlers resolve it to \ + decide the spend is final, and a zero txid makes the emit a no-op" + ); + unsafe { free_wallet_changeset_ffi(&ffi) }; + } + + /// The projection must not double-count: an account that has both a + /// surviving record and (from another record) a suppressed spend emits + /// each spend once. `unrecorded_spends` is populated only for suppressed + /// records, so the two sources are disjoint by construction — this pins + /// that they stay so. + #[test] + fn unrecorded_spends_do_not_duplicate_record_derived_spends() { + use dashcore::hashes::Hash; + use platform_wallet::changeset::UnrecordedSpend; + + let account = AccountType::DashpayExternalAccount { + index: 0, + user_identity_id: [1u8; 32], + friend_identity_id: [2u8; 32], + }; + let mut cs = CoreChangeSet::default(); + for vout in 0..3u32 { + cs.unrecorded_spends.push(UnrecordedSpend { + outpoint: dashcore::OutPoint { + txid: dashcore::Txid::from_slice(&[0x33; 32]).expect("txid"), + vout, + }, + spending_txid: dashcore::Txid::from_slice(&[0x44; 32]).expect("txid"), + account_type: account, + }); + } + + let ffi = WalletChangeSetFFI::from_changeset(&cs); + assert_eq!(ffi.accounts_count, 1, "all three share one account bucket"); + let bucket = unsafe { &*ffi.accounts }; + assert_eq!( + bucket.utxos_spent_count, 3, + "each suppressed spend emits exactly once" + ); + unsafe { free_wallet_changeset_ffi(&ffi) }; + } + /// The `has_*` flags stay false (values -1) for accounts with /// records but no watermark update this batch, so the Swift /// persister never regresses a stored value on a no-usage round. diff --git a/packages/rs-platform-wallet/src/changeset/changeset.rs b/packages/rs-platform-wallet/src/changeset/changeset.rs index fa425fbde5..54763b99f9 100644 --- a/packages/rs-platform-wallet/src/changeset/changeset.rs +++ b/packages/rs-platform-wallet/src/changeset/changeset.rs @@ -58,6 +58,26 @@ use crate::wallet::identity::{ // Core wallet changeset — projection of upstream `WalletEvent` data // --------------------------------------------------------------------------- +/// One outpoint whose spend must still be persisted even though the record +/// that spent it never reaches the persister's `records` list. +/// +/// See [`CoreChangeSet::unrecorded_spends`] for why this exists, why the +/// spending txid cannot be recovered downstream, and which backends act on +/// it today. +#[derive(Debug, Clone, PartialEq, Eq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct UnrecordedSpend { + /// The outpoint being spent — the stale row to clear. + pub outpoint: OutPoint, + /// Txid of the transaction that spends it. Both host persisters resolve + /// this to decide whether the spend is final, so it must survive. + pub spending_txid: Txid, + /// The account whose (suppressed) record spent the outpoint. Used only + /// to route the entry into a per-account bucket on the FFI surface; + /// both host handlers resolve the TXO by outpoint, wallet-wide. + pub account_type: AccountType, +} + /// Platform-owned projection of the core-wallet deltas that upstream's /// `WalletEvent` bus delivers. /// @@ -104,6 +124,47 @@ pub struct CoreChangeSet { /// `OutputRole::Change` per the upstream `TransactionRecord`). pub new_utxos: Vec, + /// Spend-clears whose spending record was deliberately **suppressed** + /// from [`Self::records`] — a contact's watch-only chain (see + /// `core_bridge`'s `is_contact_watch_only`, dashpay/platform#4363). + /// + /// [`Self::spent_utxos`] already carries these outpoints, and the + /// backend that consumes it directly (SQLite) needs nothing from this + /// field — the #4363 heal is live there. The FFI persister is the gap: + /// it derives its per-account spend lists from `records`, so a spend + /// whose only record was suppressed had nothing left to be derived + /// from and produced no account bucket and no spend entry at all. + /// + /// Reconstructing it from `spent_utxos` alone is not possible: a + /// [`Utxo`] carries no spending txid, and both host handlers key the + /// `isSpent` flip on resolving the spending transaction. So this + /// carries the two things record-suppression destroys — *which* + /// transaction did the spending, and *which* account's record it was — + /// leaving `spent_utxos` and its existing consumers untouched. + /// + /// # Status: crosses the FFI, inert on Android/iOS today + /// + /// This field closes the *Rust-side* gap only. Both mobile handlers + /// resolve `spending_txid` to a persisted transaction row before they + /// will touch `isSpent` (`getByTxid(spendingTxid)` on Android, the + /// `PersistentTransaction` fetch on iOS), and for a suppressed record + /// that row is precisely what never gets written. So a pure + /// contact → third-party spend still does not flip `isSpent` on either + /// host: the entry arrives, the lookup misses, and the handler leaves + /// the row alone. The mixed case — the contact spends the stale coin in + /// a transaction that also carries a surviving record of ours — is + /// already healed without this field, because that record's FFI emit + /// carries `input_outpoints` for *every* input of the transaction and + /// the hosts reconcile the spend from there. + /// + /// The plumbing stays because it is the half that cannot be done + /// downstream, and it becomes load-bearing the moment either of the two + /// planned pieces lands: a host-visible "this spend is final, no + /// transaction row is coming" contract on `SpentOutPointFFI`, or the + /// store-reconciliation pass that heals stale rows out of band. Until + /// then, do not describe the mobile heal as working. + pub unrecorded_spends: Vec, + /// InstantSend locks observed for records that are NOT yet in a /// chain-locked block (i.e. records still in `Mempool`, /// `InstantSend`, or `InBlock` context — anything `InChainLockedBlock` @@ -240,6 +301,7 @@ impl Merge for CoreChangeSet { self.records.extend(other.records); self.spent_utxos.extend(other.spent_utxos); self.new_utxos.extend(other.new_utxos); + self.unrecorded_spends.extend(other.unrecorded_spends); // IS-lock map: last-write-wins per txid. A second IS-lock for // the same txid (e.g. a follow-up event re-confirming the lock) @@ -338,6 +400,7 @@ impl Merge for CoreChangeSet { self.records.is_empty() && self.spent_utxos.is_empty() && self.new_utxos.is_empty() + && self.unrecorded_spends.is_empty() && self.instant_locks_for_non_final_records.is_empty() && self.last_processed_height.is_none() && self.synced_height.is_none() diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs index df1b4701cf..6009bde7de 100644 --- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs +++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs @@ -52,7 +52,7 @@ use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; use crate::changeset::changeset::{ - AssetLockChangeSet, CoreChangeSet, HighestUsedIndexes, PlatformWalletChangeSet, + AssetLockChangeSet, CoreChangeSet, HighestUsedIndexes, PlatformWalletChangeSet, UnrecordedSpend, }; use crate::changeset::merge::Merge; use crate::changeset::traits::PlatformWalletPersistence; @@ -618,6 +618,14 @@ async fn build_core_changeset( CoreChangeSet { new_utxos: derive_new_utxos(record), spent_utxos: derive_spent_utxos(record), + // Suppressed below but still spending: keep the outpoint, + // its spender and its account alive for the FFI persister, + // which projects per account off `records` and would + // otherwise never see this spend at all. First sighting + // only — the `BlockProcessed` arm re-derives on the + // confirming phase, which is the emit the hosts' in-block + // gate actually needs. + unrecorded_spends: derive_unrecorded_spends(record), // A contact's watch-only chain never defines the // wallet's transaction row (see `is_contact_watch_only`). // The usage deltas below are still emitted, so the @@ -672,6 +680,31 @@ async fn build_core_changeset( cs.new_utxos.extend(derive_new_utxos(r)); cs.spent_utxos.extend(derive_spent_utxos(r)); } + // Suppressed-record spends track `records`, NOT `spent_utxos`. + // + // The UTXO topology vecs above are correctly `inserted`-only: a + // confirmation re-emit changes no outpoint's existence, and + // re-deriving them would re-delete and re-add rows for nothing. + // `unrecorded_spends` is a different kind of thing — it is the + // stand-in for the FFI emit a *record* would have produced, and + // records re-emit from all three phases (see `cs.records` + // below). Deriving it from `inserted` alone reproduced, for + // suppressed records, exactly the bug the phase-chaining on + // `cs.records` exists to avoid: a spend first sighted in the + // mempool got one emit, at a context that can never satisfy the + // hosts' `context >= IN_BLOCK` gate on the `isSpent` flip, and + // then nothing when the block that confirmed it arrived. A + // surviving record gets a fresh emit at every new context; a + // suppressed one must too. + // + // No double-count: within one `BlockProcessed` a record appears + // in exactly one of the three lists, and `derive_unrecorded_spends` + // is non-empty only for records the filter below drops — so this + // stays disjoint from the spends the FFI builder re-derives from + // `cs.records`. + for r in inserted.iter().chain(updated.iter()).chain(matured.iter()) { + cs.unrecorded_spends.extend(derive_unrecorded_spends(r)); + } // Updated records (re-confirmation, IS-lock applied to a known // mempool tx, etc.) don't usually change UTXO topology — the // record's content does change though, so re-emit it. @@ -1072,6 +1105,38 @@ fn derive_new_utxos(record: &TransactionRecord) -> Vec { .collect() } +/// Derive the spend-clears that `records` suppression would otherwise lose. +/// +/// Empty for every record that *does* reach the persister's `records` list — +/// those spends are re-derived per account from the record itself on the FFI +/// surface, and emitting them twice would duplicate the entry. Non-empty only +/// for a contact's watch-only record (see [`is_contact_watch_only`]), which is +/// dropped from `records` while its spends must still clear the stale rows a +/// pre-#4363 build wrote. +/// +/// Pairs each outpoint with the spending txid and the owning account, the two +/// facts [`CoreChangeSet::unrecorded_spends`] exists to preserve. Read that +/// field's docs before relying on this: the entries cross the FFI, but neither +/// mobile handler acts on them yet, so this is plumbing for a heal that is +/// live on SQLite and still dormant on Android/iOS. +fn derive_unrecorded_spends(record: &TransactionRecord) -> Vec { + if !is_contact_watch_only(record) { + return Vec::new(); + } + record + .input_details + .iter() + .filter_map(|detail| { + let input = record.transaction.input.get(detail.index as usize)?; + Some(UnrecordedSpend { + outpoint: input.previous_output, + spending_txid: record.txid, + account_type: record.account_type, + }) + }) + .collect() +} + /// Derive the "ours" UTXOs spent by a transaction's inputs. /// /// Walks `record.input_details` (the entries keyed to inputs that spent @@ -1118,6 +1183,7 @@ impl CoreChangeSet { self.records.is_empty() && self.spent_utxos.is_empty() && self.new_utxos.is_empty() + && self.unrecorded_spends.is_empty() && self.instant_locks_for_non_final_records.is_empty() && self.last_processed_height.is_none() && self.synced_height.is_none() @@ -1250,18 +1316,19 @@ mod contact_watch_only_projection_tests { } #[allow(clippy::too_many_arguments)] - fn record( + fn record_at( tx: &Transaction, account_type: AccountType, direction: TransactionDirection, input_details: Vec, output_details: Vec, net_amount: i64, + context: TransactionContext, ) -> TransactionRecord { TransactionRecord::new( tx.clone(), account_type, - in_block(1_000), + context, TransactionType::Standard, direction, input_details, @@ -1270,6 +1337,57 @@ mod contact_watch_only_projection_tests { ) } + #[allow(clippy::too_many_arguments)] + fn record( + tx: &Transaction, + account_type: AccountType, + direction: TransactionDirection, + input_details: Vec, + output_details: Vec, + net_amount: i64, + ) -> TransactionRecord { + record_at( + tx, + account_type, + direction, + input_details, + output_details, + net_amount, + in_block(1_000), + ) + } + + /// The record a contact's spend of a *stale pre-#4363* TXO produces, at + /// the given context. + /// + /// The spent outpoint is [`funding_outpoint`] — the row a pre-fix build + /// wrongly wrote into our TXO set when it let the contact's watch-only + /// record define the transaction. The contact can spend it; we cannot. + fn contact_spend_of_a_stale_txo( + context: TransactionContext, + ) -> (Transaction, TransactionRecord) { + let tx = tx_with(&[(&contact_address(), PAID_TO_CONTACT)]); + let spend = record_at( + &tx, + contact_external_account(), + TransactionDirection::Outgoing, + vec![InputDetail { + index: 0, + value: PAID_TO_CONTACT, + address: contact_address(), + }], + vec![output( + 0, + OutputRole::Sent, + &contact_address(), + PAID_TO_CONTACT, + )], + -(PAID_TO_CONTACT as i64), + context, + ); + (tx, spend) + } + /// The record pair a payment to a contact really produces: the /// funding account sees `Outgoing` with a negative net, and the /// contact's watch-only chain independently sees `Incoming` with a @@ -1558,24 +1676,7 @@ mod contact_watch_only_projection_tests { /// new-TXO projection are suppressed. #[tokio::test] async fn contact_spend_still_clears_a_stale_pre_fix_txo() { - let tx = tx_with(&[(&contact_address(), PAID_TO_CONTACT)]); - let watch_only_spend = record( - &tx, - contact_external_account(), - TransactionDirection::Outgoing, - vec![InputDetail { - index: 0, - value: PAID_TO_CONTACT, - address: contact_address(), - }], - vec![output( - 0, - OutputRole::Sent, - &contact_address(), - PAID_TO_CONTACT, - )], - -(PAID_TO_CONTACT as i64), - ); + let (tx, watch_only_spend) = contact_spend_of_a_stale_txo(in_block(1_000)); let cs = build_core_changeset(&test_manager(), &block_processed(vec![watch_only_spend])).await; @@ -1589,6 +1690,152 @@ mod contact_watch_only_projection_tests { "the stale pre-fix TXO must still be removed" ); assert_eq!(cs.spent_utxos[0].outpoint, funding_outpoint()); + + // `spent_utxos` alone only heals the backends that read it directly + // (SQLite), where the #4363 heal is already live. The FFI persister + // projects per account off `records`, which is empty here, so the + // clear needs the suppressed record's spend carried explicitly — + // with the spending txid, which a `Utxo` does not have and which + // both host handlers require. That gets the entry across the FFI; + // acting on it still needs the host-side piece described on + // `CoreChangeSet::unrecorded_spends`. + assert_eq!( + cs.unrecorded_spends.len(), + 1, + "the suppressed record's spend must be carried for the FFI persister" + ); + let unrecorded = &cs.unrecorded_spends[0]; + assert_eq!(unrecorded.outpoint, funding_outpoint()); + assert_eq!( + unrecorded.spending_txid, + tx.txid(), + "the spending txid must be the contact's spending transaction" + ); + assert_eq!(unrecorded.account_type, contact_external_account()); + } + + /// The converse: a record that *survives* into `records` must NOT also + /// appear in `unrecorded_spends`. The FFI builder re-derives spends from + /// every surviving record, so emitting both would double-count the clear. + #[tokio::test] + async fn a_surviving_record_emits_no_unrecorded_spend() { + let (_, funding, _) = contact_payment_records(); + let cs = build_core_changeset(&test_manager(), &transaction_detected(funding)).await; + + assert_eq!(cs.records.len(), 1, "the funding record still persists"); + assert!( + cs.unrecorded_spends.is_empty(), + "a record the persister will see must not also be carried as an \ + unrecorded spend — the FFI builder would emit its inputs twice" + ); + } + + /// A suppressed spend first sighted in the **mempool** must re-emit when + /// its block confirmation arrives. + /// + /// Both host handlers gate the `isSpent` flip on the spending transaction + /// having reached `context >= IN_BLOCK`, so a mempool-context emit can + /// never satisfy them on its own. Surviving records get a fresh emit from + /// every record-carrying phase — `inserted`, `updated` and `matured` all + /// feed `cs.records` — but `unrecorded_spends` was derived from `inserted` + /// alone, tracking the UTXO-topology vecs instead. A suppressed record + /// therefore got exactly one emit, forever at mempool context, and the + /// `updated` phase that confirmed it carried nothing at all. + /// + /// Pre-fix, phase 2 below asserted `left: 0, right: 1`. + #[tokio::test] + async fn a_suppressed_spend_re_emits_when_its_block_confirmation_arrives() { + // Phase 1 — mempool sighting, delivered as `TransactionDetected`. + let (tx, mempool_spend) = contact_spend_of_a_stale_txo(TransactionContext::Mempool); + let cs = build_core_changeset(&test_manager(), &transaction_detected(mempool_spend)).await; + + assert!( + cs.records.is_empty(), + "the contact's record is suppressed on first sighting" + ); + assert_eq!( + cs.unrecorded_spends.len(), + 1, + "the first sighting emits — but at a context no host will act on" + ); + + // Phase 2 — the block that confirms it. The record already exists, so + // upstream re-emits it under `updated` (its context advanced), never + // under `inserted`. + let (_, confirmed_spend) = contact_spend_of_a_stale_txo(in_block(1_000)); + let event = WalletEvent::BlockProcessed { + wallet_id: WALLET_ID, + height: 1_000, + chain_lock: None, + inserted: vec![], + updated: vec![confirmed_spend], + matured: vec![], + balance: WalletCoreBalance::default(), + account_balances: BTreeMap::new(), + addresses_derived: vec![], + }; + let cs = build_core_changeset(&test_manager(), &event).await; + + assert!( + cs.records.is_empty(), + "confirmation does not un-suppress the contact's record" + ); + assert!( + cs.spent_utxos.is_empty(), + "the UTXO-topology vecs stay `inserted`-only — a confirmation \ + re-emit destroys and creates no outpoint" + ); + assert_eq!( + cs.unrecorded_spends.len(), + 1, + "the confirming phase must re-emit the spend — it is the only \ + emit that can ever satisfy the hosts' in-block gate" + ); + let unrecorded = &cs.unrecorded_spends[0]; + assert_eq!(unrecorded.outpoint, funding_outpoint()); + assert_eq!( + unrecorded.spending_txid, + tx.txid(), + "the spending txid must survive the re-emit" + ); + assert_eq!(unrecorded.account_type, contact_external_account()); + } + + /// Chaining the phases must not blur the two spend sources: a block that + /// confirms a contact's suppressed spend while also inserting a + /// transaction of ours yields one unrecorded spend (the contact's) and one + /// record (ours), and the unrecorded entry is routed to the contact's + /// account — not the funding account whose record the FFI builder will + /// re-derive spends from itself. + #[tokio::test] + async fn phase_chaining_keeps_suppressed_and_surviving_spends_disjoint() { + let (_, funding, _) = contact_payment_records(); + let (_, confirmed_contact_spend) = contact_spend_of_a_stale_txo(in_block(1_000)); + let event = WalletEvent::BlockProcessed { + wallet_id: WALLET_ID, + height: 1_000, + chain_lock: None, + inserted: vec![funding], + updated: vec![confirmed_contact_spend], + matured: vec![], + balance: WalletCoreBalance::default(), + account_balances: BTreeMap::new(), + addresses_derived: vec![], + }; + let cs = build_core_changeset(&test_manager(), &event).await; + + assert_eq!(cs.records.len(), 1, "only our own record defines a row"); + assert_eq!(cs.records[0].account_type, bip44_account_0()); + assert_eq!( + cs.unrecorded_spends.len(), + 1, + "our inserted record contributes no unrecorded spend" + ); + assert_eq!( + cs.unrecorded_spends[0].account_type, + contact_external_account(), + "the entry is routed to the suppressed record's account" + ); } } diff --git a/packages/rs-platform-wallet/src/changeset/mod.rs b/packages/rs-platform-wallet/src/changeset/mod.rs index e87cc14aee..88a04bbc2c 100644 --- a/packages/rs-platform-wallet/src/changeset/mod.rs +++ b/packages/rs-platform-wallet/src/changeset/mod.rs @@ -36,7 +36,7 @@ pub use changeset::{ PendingContactCryptoKind, PendingContactCryptoOp, PlatformAddressBalanceEntry, PlatformAddressChangeSet, PlatformWalletChangeSet, ProviderKeyAccountEntry, ProviderKeyExtendedPubKey, ProviderPlatformNodePubKey, ReceivedContactRequestKey, - SentContactRequestKey, TokenBalanceChangeSet, WalletMetadataEntry, + SentContactRequestKey, TokenBalanceChangeSet, UnrecordedSpend, WalletMetadataEntry, }; pub use client_start_state::ClientStartState; pub use client_wallet_start_state::ClientWalletStartState; 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 c084ea667a..b2f5a11572 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, funding_accounts) = { + let (payment_address, used_flip_changeset, tx, fee, funding_accounts, reservation_token) = { let mut wm = self.wallet_manager.write().await; // Resolve the external account's xpub so we can derive addresses. @@ -1269,13 +1269,37 @@ impl DashPayView<'_, B> { // `impl TransactionSigner for S`) rather than the // resident `wallet`, so funding-input signatures are produced // from Keychain-derived keys without a resident seed. - // `build_signed` returns the fee the transaction actually - // pays — Σ(selected input values) − Σ(output values), a + // `build_signed_reserved` returns the fee the transaction + // actually pays — Σ(selected input values) − Σ(output values), a // dropped sub-dust change remainder included — since // rust-dashcore#872 (pinned above). No caller-side // recomputation needed. - let (tx, fee) = match builder - .build_signed(signer, |addr| funding_paths.get(&addr).cloned()) + // + // `build_signed_reserved` rather than `build_signed`: both reserve + // the selected inputs identically (upstream `build_signed` is a + // thin wrapper that *discards* the token), but only this one hands + // back the `ReservationToken` stamped onto them. Every failure + // path after this point has to reconcile that reservation, and two + // of them cannot do it correctly without the token: + // + // * the `persister.store` failure below aborts pre-broadcast + // while the inputs of a *fully signed* transaction stay + // reserved — with no token there was nothing to release with, + // so they were stranded until the TTL backstop and an + // immediate retry failed with spurious insufficient funds; + // * the rejected-broadcast release runs after two `.await`s, so + // key-wallet's TTL sweep may already have reclaimed this + // build's reservation and a *newer* build re-taken the same + // outpoints. The unconditional by-outpoint release would then + // clobber that newer owner and re-expose the inputs of a + // transaction that may have been sent (`dashpay/platform#4185`, + // and see `release_reservation_after_rejected_broadcast`). + // + // Since #4373 pooled the funding across BIP44 + BIP32 + every + // DashPay receiving account, the blast radius of both is the + // wallet's whole spendable set rather than one account. + let (tx, fee, reservation_token) = match builder + .build_signed_reserved(signer, |addr| funding_paths.get(&addr).cloned()) .await { Ok(built) => built, @@ -1295,6 +1319,11 @@ impl DashPayView<'_, B> { // reconstruction. Used indices must chain within the // gap limit, so consumption is committed only once a // fully signed transaction exists. + // + // No reservation survives this arm, so there is nothing to + // release here: selection failing never reserved, and a + // signer failing is released owner-guarded inside + // `build_signed_reserved` before it returns the error. if let Some(external_account) = info .core_wallet .accounts @@ -1313,6 +1342,7 @@ impl DashPayView<'_, B> { tx, fee, offered_accounts, + reservation_token, ) }; @@ -1327,11 +1357,31 @@ impl DashPayView<'_, B> { // leaves a one-address gap that the pool's gap window absorbs on // retry — bounded, because a signed transaction exists here, unlike // the unbounded build-failure case rolled back above. - self.persister.store(used_flip_changeset).map_err(|e| { - PlatformWalletError::Persistence(format!( + // + // The abort must also hand the build's reserved inputs back. This + // transaction is fully signed, so `build_signed_reserved` left every + // selected input reserved across each contributing account expecting a + // broadcast that now never happens; returning `?` without releasing + // stranded them until the reservation TTL expired, and — since #4373 + // pooled the funding — that is the whole spendable set, not one + // account. The user's retry would fail with a spurious + // insufficient-funds until the backstop fired. Release owner-guarded: + // the store call is an `.await`-free but fallible step reached after + // the build's own `.await`, so a TTL sweep may already have handed + // these outpoints to a newer build that must not be clobbered. + if let Err(e) = self.persister.store(used_flip_changeset) { + crate::wallet::reservations::release_build_reservation( + &self.wallet_manager, + &self.wallet_id, + &funding_accounts, + &tx, + reservation_token, + ) + .await; + return Err(PlatformWalletError::Persistence(format!( "failed to persist payment-address used flip: {e}" - )) - })?; + ))); + } // --- 3. Broadcast the transaction, releasing the build's UTXO // reservation if the broadcast is definitively rejected pre-send. --- @@ -1347,9 +1397,14 @@ impl DashPayView<'_, B> { &self.wallet_id, &funding_accounts, &tx, - // This path does not thread the build's reservation token - // either; keep the historical unconditional release. - None, + // Owner-guarded with the build's own token. The release + // runs after the build and the broadcast have both + // `.await`ed, so the TTL sweep may have reclaimed this + // reservation and a newer build re-reserved the same + // outpoints; the historical `None` took the unconditional + // branch and freed *that* build's inputs, re-exposing the + // inputs of a transaction that may since have been sent. + reservation_token, ) .await; Err(e) @@ -1519,6 +1574,10 @@ mod tests { #[derive(Default)] struct RecordingPersister { stores: Mutex>, + /// Arms every subsequent `store` to fail, for the pre-broadcast + /// persistence-abort paths. Default `false`, so every existing test + /// keeps the always-succeeding behaviour. + fail_stores: Mutex, } impl PlatformWalletPersistence for RecordingPersister { @@ -1527,6 +1586,9 @@ mod tests { wallet_id: WalletId, changeset: PlatformWalletChangeSet, ) -> Result<(), PersistenceError> { + if *self.fail_stores.lock().unwrap() { + return Err(PersistenceError::backend("injected store failure")); + } self.stores.lock().unwrap().push((wallet_id, changeset)); Ok(()) } @@ -6441,6 +6503,139 @@ mod tests { ); } + /// A `persister.store` failure aborts the send pre-broadcast holding a + /// **fully signed** transaction, so it must hand the build's reserved + /// inputs back. + /// + /// Pre-fix `send_payment` called `build_signed`, which reserves the + /// selected inputs exactly like `build_signed_reserved` but discards the + /// token, and the used-flip store propagated its error with `?` — no + /// release of any kind. The inputs of a transaction that will never be + /// broadcast stayed reserved until the TTL backstop, and since #4373 + /// pooled the funding that is the whole spendable set. + /// + /// The retry is the assertion: the wallet holds a single UTXO, so the + /// second send can only build if the first one's reservation was + /// released. Against the pre-fix code it fails with insufficient funds. + #[tokio::test] + async fn send_payment_store_failure_releases_the_build_reservation() { + 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(); + + // Exactly one spendable UTXO: a leaked reservation is therefore + // indistinguishable from an empty wallet on the retry. + fund_bip44_account_0(&manager, wallet_id, 0xD1, 120_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); + + // Arm the store to fail so the send aborts on the used-flip persist, + // after build + sign have reserved the input and before any broadcast. + *persister.fail_stores.lock().unwrap() = true; + + let sending = with_accepting_broadcaster(iw); + let err = sending + .dashpay() + .send_payment(&owner_id, &contact_id, 50_000, None, &signer, &provider) + .await + .expect_err("the armed store failure must abort the send"); + assert!( + matches!(err, PlatformWalletError::Persistence(_)), + "the send must abort on the used-flip persist (so the input was \ + already reserved), got: {err:?}" + ); + + // Disarm: the retry must be able to persist its own flip. + *persister.fail_stores.lock().unwrap() = false; + + sending + .dashpay() + .send_payment(&owner_id, &contact_id, 50_000, None, &signer, &provider) + .await + .expect( + "an immediate retry must reselect the input — a store failure that \ + returns without releasing strands the wallet's whole spendable set \ + until the reservation TTL backstop fires", + ); + } + + /// The rejected-broadcast release must be **owner-guarded**, so it cannot + /// clobber a newer build's reservation of the same outpoints. + /// + /// The release runs after two `.await`s (build, then broadcast), so + /// key-wallet's TTL sweep can reclaim this build's reservation and a newer + /// build re-reserve the same outpoint in between. Pre-fix `send_payment` + /// passed `reservation_token = None`, taking + /// `release_reservation_after_rejected_broadcast`'s unconditional + /// by-outpoint branch — which frees whatever holds the outpoint *now*, + /// including that newer owner, re-exposing the inputs of a transaction + /// that may since have been sent (`dashpay/platform#4185`). + /// + /// [`SweepingRejectingBroadcaster`] reproduces exactly that interleaving + /// inside the broadcast await. The assertion is that the newer + /// reservation *survives*: a later build must still be unable to select + /// the outpoint. Against the pre-fix `None` the clobber frees it and that + /// build succeeds. + #[tokio::test] + async fn send_payment_rejected_broadcast_release_is_owner_guarded() { + 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(); + + fund_bip44_account_0(&manager, wallet_id, 0xD2, 120_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); + persister.stores.lock().unwrap().clear(); + + let racing = with_sweeping_rejecting_broadcaster(iw); + let err = racing + .dashpay() + .send_payment(&owner_id, &contact_id, 50_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, got: {err:?}" + ); + assert!( + *racing.broadcaster.re_reserved.lock().unwrap(), + "the broadcaster must have swept and re-reserved the outpoint under a \ + NEW token — without that interleaving the test proves nothing" + ); + + // The newer reservation must still hold the wallet's only UTXO, so a + // fresh build has nothing to select. + let retry = with_accepting_broadcaster(iw); + let retry_err = retry + .dashpay() + .send_payment(&owner_id, &contact_id, 50_000, None, &signer, &provider) + .await + .expect_err( + "the rejection released with the OLD token must leave the newer \ + build's reservation intact — an unconditional release clobbers it \ + and lets this build re-select an outpoint another transaction owns", + ); + assert!( + matches!(retry_err, PlatformWalletError::TransactionBuild(_)), + "the outpoint must still be reserved (build-time insufficient funds), \ + got: {retry_err:?}" + ); + } + /// The `send_payment` used-flag flip persist must run only AFTER the /// wallet-manager write guard is released (and before the broadcast). /// @@ -6661,6 +6856,122 @@ mod tests { } } + /// Rejecting broadcaster that reproduces the reservation TTL race inside + /// the broadcast await: before answering `Rejected` it (1) releases the + /// in-flight build's reservation unconditionally — what key-wallet's TTL + /// sweep does when a build outlives the reservation window — and (2) lets + /// a *newer* build re-reserve the very same outpoints under a fresh + /// token. + /// + /// `send_payment`'s post-rejection cleanup therefore runs against an + /// outpoint owned by someone else, which is the whole point of releasing + /// owner-guarded. + struct SweepingRejectingBroadcaster { + wallet_manager: Arc< + tokio::sync::RwLock< + key_wallet_manager::WalletManager< + crate::wallet::platform_wallet::PlatformWalletInfo, + >, + >, + >, + wallet_id: WalletId, + /// Set once the re-reservation under a new token actually happened, + /// so the test can assert it raced rather than silently no-op'd. + re_reserved: Mutex, + } + + #[async_trait::async_trait] + impl crate::broadcaster::TransactionBroadcaster for SweepingRejectingBroadcaster { + async fn broadcast( + &self, + transaction: &dashcore::Transaction, + ) -> Result { + use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; + use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder; + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; + + { + // `send_payment` drops the manager write guard before it + // broadcasts, so this cannot deadlock. + let mut wm = self.wallet_manager.write().await; + let height = wm + .get_wallet_info(&self.wallet_id) + .expect("wallet info") + .core_wallet + .last_processed_height(); + let (wallet, info) = wm + .get_wallet_and_info_mut(&self.wallet_id) + .expect("wallet and info"); + + let managed = info + .core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("BIP-44 managed account 0"); + // (1) The TTL sweep reclaims the in-flight build's inputs. + managed.release_reservation(transaction); + + // Reuse one of the account's own derived addresses as the + // output — this build exists only to take the reservation. + let sink = managed + .managed_account_type() + .address_pools() + .first() + .expect("address pool") + .addresses + .values() + .next() + .expect("derived address") + .address + .clone(); + let account = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .expect("BIP-44 account 0"); + + // (2) A newer build re-reserves the same outpoint under a new + // token. Holding the returned token is unnecessary — the test + // asserts the reservation survives, not who owns it. + let (_tx, _fee, token) = TransactionBuilder::new() + .set_current_height(height) + .set_selection_strategy(SelectionStrategy::LargestFirst) + .add_funding(managed, account) + .add_output(&sink, 10_000) + .build_unsigned_reserved() + .expect("the swept outpoint must be selectable again"); + *self.re_reserved.lock().unwrap() = token.is_some(); + } + + Err(crate::broadcaster::BroadcastError::Rejected { + reason: "test rejection after reservation sweep".to_string(), + }) + } + } + + /// [`with_rejecting_broadcaster`], but the transport sweeps and + /// re-reserves the build's inputs before rejecting. + fn with_sweeping_rejecting_broadcaster( + real: &crate::wallet::identity::IdentityWallet, + ) -> crate::wallet::identity::IdentityWallet { + crate::wallet::identity::IdentityWallet { + sdk: Arc::clone(&real.sdk), + wallet_manager: Arc::clone(&real.wallet_manager), + wallet_id: real.wallet_id, + asset_locks: Arc::clone(&real.asset_locks), + persister: real.persister.clone(), + broadcaster: Arc::new(SweepingRejectingBroadcaster { + wallet_manager: Arc::clone(&real.wallet_manager), + wallet_id: real.wallet_id, + re_reserved: Mutex::new(false), + }), + sdk_writer: Arc::clone(&real.sdk_writer), + dpns_operation_gate: Arc::clone(&real.dpns_operation_gate), + dpns_sync_progress: Arc::clone(&real.dpns_sync_progress), + } + } + /// [`with_accepting_broadcaster`], but the transport definitively /// rejects. fn with_rejecting_broadcaster( diff --git a/packages/rs-platform-wallet/src/wallet/reservations.rs b/packages/rs-platform-wallet/src/wallet/reservations.rs index dc95dce484..f23d3f066b 100644 --- a/packages/rs-platform-wallet/src/wallet/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/reservations.rs @@ -95,6 +95,42 @@ pub(crate) async fn release_reservation_after_rejected_broadcast( funding_accounts: &[AccountType], tx: &Transaction, reservation_token: Option, +) { + release_build_reservation( + wallet_manager, + wallet_id, + funding_accounts, + tx, + reservation_token, + ) + .await +} + +/// Release the funding accounts' UTXO reservations for a built transaction the +/// caller has decided not to send. +/// +/// The general form of [`release_reservation_after_rejected_broadcast`]: same +/// per-account, owner-guarded reconciliation, for aborts that are not a +/// rejected broadcast. The other pre-broadcast abort is a persistence failure +/// — `send_payment` must durably record its payment-address used flip *before* +/// broadcasting, and when that store fails the send is abandoned with a fully +/// signed transaction in hand whose inputs are all still reserved. +/// +/// `funding_accounts` are the accounts that contributed inputs to `tx` — the +/// accounts handed to `add_funding` when it was built. A build funded from +/// several of them (a pooled asset lock, a pooled send) reserves in **each** +/// account's own `ReservationSet` under the one token, so every one of them +/// must reconcile; releasing only the first would leave the rest of the inputs +/// held until the TTL backstop reclaims them. +/// +/// Pass the build's [`ReservationToken`](key_wallet::ReservationToken) +/// whenever it is available — see the owner-guard rationale inline below. +pub(crate) async fn release_build_reservation( + wallet_manager: &RwLock>, + wallet_id: &WalletId, + funding_accounts: &[AccountType], + tx: &Transaction, + reservation_token: Option, ) { // `release_reservation` takes `&self` and the manager map is // untouched, so a read lock suffices — this cleanup does not @@ -104,7 +140,7 @@ pub(crate) async fn release_reservation_after_rejected_broadcast( tracing::warn!( wallet_id = %hex::encode(wallet_id), ?funding_accounts, - "could not release UTXO reservation after rejected broadcast: wallet not found" + "could not release the build's UTXO reservation: wallet not found" ); return; }; @@ -124,7 +160,7 @@ pub(crate) async fn release_reservation_after_rejected_broadcast( None => tracing::warn!( wallet_id = %hex::encode(wallet_id), ?funding_account, - "could not release UTXO reservation after rejected broadcast: \ + "could not release the build's UTXO reservation: \ funds account not found" ), }