From afe26111249b58a786d8a4edb960f29ac1a4ba6c Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sun, 5 Jul 2026 20:10:23 -0500 Subject: [PATCH 01/12] fix(platform-wallet): close read-before-broadcast race in resume_asset_lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resume_asset_lock`'s `Built` arm snapshotted the tracked row under a read lock, dropped it, then awaited `broadcaster.broadcast(&tx)` before advancing the status to `Broadcast`. During that window a concurrent `create_funded_asset_lock_proof` that received `Rejected` from its own broadcast would see the row still at `Built`, remove it, and release the funding reservation — while resume was still handing the same transaction to the network. Advance the row to `Broadcast` under the write lock BEFORE calling `broadcast`, so `untrack_asset_lock`'s guard fires (row + reservation preserved) or the advance itself fails and resume returns before broadcasting. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/wallet/asset_lock/build.rs | 337 ++++++++++++++++++ .../src/wallet/asset_lock/sync/recovery.rs | 31 +- 2 files changed, 365 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 9d704683761..7c30b47c9ed 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -604,7 +604,9 @@ impl AssetLockManager { #[cfg(test)] mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; + use std::time::Duration; use dashcore::OutPoint; use key_wallet::account::account_type::StandardAccountType; @@ -1378,4 +1380,339 @@ mod tests { } } } + + /// Broadcaster that models the read-before-broadcast interleave between + /// the create-path Rejected cleanup and a concurrent `resume_asset_lock` + /// re-broadcast. Call 1 is create's broadcast (blocks until the test + /// releases it, then returns `Rejected`); call 2 is resume's re-broadcast + /// (blocks until the test releases it, then returns success). Each side + /// signals a `Notify` when it enters so the test can order the race + /// deterministically. + struct RaceRejectDuringResumeBroadcaster { + call_count: AtomicUsize, + create_entered: Arc, + create_can_return: Arc, + resume_entered: Arc, + resume_can_return: Arc, + } + + #[async_trait] + impl TransactionBroadcaster for RaceRejectDuringResumeBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + let call = self.call_count.fetch_add(1, Ordering::SeqCst); + if call == 0 { + self.create_entered.notify_one(); + self.create_can_return.notified().await; + Err(BroadcastError::Rejected { + reason: "simulated rejection during concurrent resume".to_string(), + }) + } else { + self.resume_entered.notify_one(); + self.resume_can_return.notified().await; + Ok(transaction.txid()) + } + } + } + + /// The read-before-broadcast interleave: a `resume_asset_lock` snapshots + /// the `Built` row under a read lock, drops the lock, and calls + /// `broadcast(&tx)` while the create path is still awaiting its own + /// broadcast. When the create broadcast then returns `Rejected`, the + /// cleanup must not remove the row or release the funding reservation — + /// the resume path may still be handing the same transaction to the + /// network. The `Built`-arm advance-before-broadcast in + /// `resume_asset_lock` closes this window by pushing the status past + /// `Built` under the write lock before the re-broadcast; the untrack + /// guard then preserves the row and its reservation. + #[tokio::test] + async fn rejected_broadcast_racing_resume_read_before_broadcast_keeps_row_and_reservation() { + let (wallet_manager, wallet_id, _balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + + let create_entered = Arc::new(Notify::new()); + let create_can_return = Arc::new(Notify::new()); + let resume_entered = Arc::new(Notify::new()); + let resume_can_return = Arc::new(Notify::new()); + + let broadcaster = Arc::new(RaceRejectDuringResumeBroadcaster { + call_count: AtomicUsize::new(0), + create_entered: Arc::clone(&create_entered), + create_can_return: Arc::clone(&create_can_return), + resume_entered: Arc::clone(&resume_entered), + resume_can_return: Arc::clone(&resume_can_return), + }); + let persistence = Arc::new(CapturingPersistence::default()); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let manager = Arc::new(AssetLockManager::new( + sdk, + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(Notify::new()), + Arc::clone(&broadcaster), + WalletPersister::new( + wallet_id, + Arc::clone(&persistence) as Arc, + ), + )); + + // 1. Start the create path. It builds a fresh asset-lock tx, tracks + // the `Built` row, then blocks inside our broadcaster on the + // first call. + let signer = Arc::new(signer); + let manager_create = Arc::clone(&manager); + let signer_create = Arc::clone(&signer); + let create_handle = tokio::spawn(async move { + manager_create + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &*signer_create, + ) + .await + }); + create_entered.notified().await; + + // 2. The row is now tracked at `Built`; snapshot its outpoint. + let out_point = { + let wm = wallet_manager.read().await; + let (_, info) = wm.get_wallet_and_info(&wallet_id).expect("wallet present"); + *info + .tracked_asset_locks + .keys() + .next() + .expect("built row tracked before broadcast") + }; + + // 3. Start the resume path against the same outpoint. It looks up + // the tracked row under a read lock, drops the lock, and — with + // the fix — advances Built → Broadcast under the write lock + // before entering the second broadcaster call. + let manager_resume = Arc::clone(&manager); + let resume_handle = tokio::spawn(async move { + manager_resume + .resume_asset_lock(&out_point, Some(Duration::from_millis(50))) + .await + }); + resume_entered.notified().await; + + // 4. Let the create broadcast return `Rejected`. Because the row + // has already been advanced past `Built` by the concurrent + // resume, the untrack guard must refuse to remove it and the + // reservation must stay held. + create_can_return.notify_one(); + let create_result = create_handle.await.expect("create task joined"); + assert!( + matches!( + create_result, + Err(PlatformWalletError::TransactionBroadcast(_)) + ), + "rejection should still surface, got {create_result:?}" + ); + + // 5. Let the resume broadcast complete. Resume will then wait for a + // proof and time out (short deadline), which is fine — the + // interleave under test is already resolved by this point. + resume_can_return.notify_one(); + let _ = resume_handle.await.expect("resume task joined"); + + // Both sides actually attempted a broadcast: the interleave really + // did happen (resume did not error out before its broadcast call). + assert_eq!( + broadcaster.call_count.load(Ordering::SeqCst), + 2, + "both create and resume should have attempted a broadcast" + ); + + // The row survives the rejection cleanup, past `Built`. + { + let wm = wallet_manager.read().await; + let (_, info) = wm.get_wallet_and_info(&wallet_id).expect("wallet present"); + assert_eq!( + info.tracked_asset_locks.len(), + 1, + "row must survive: a concurrent resume broadcast the tx, so the \ + rejection cleanup must not delete the row" + ); + let lock = info + .tracked_asset_locks + .get(&out_point) + .expect("row kept at the raced outpoint"); + assert_ne!( + lock.status, + AssetLockStatus::Built, + "resume must have advanced the row past Built before its \ + broadcast; found {:?}", + lock.status + ); + } + + // No persisted-row deletion was queued. + assert!( + persistence.removed_outpoints().is_empty(), + "advanced row must not be queued for deletion, got {:?}", + persistence.removed_outpoints() + ); + + // The reservation was NOT released: a fresh build cannot reselect + // the single reserved UTXO — otherwise resume would have handed the + // network a transaction whose inputs are re-spendable locally. + let rebuild = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &*signer, + ) + .await; + assert!( + matches!(rebuild, Err(PlatformWalletError::AssetLockTransaction(_))), + "rebuild must fail at input selection while the reservation is \ + held across the concurrent resume, got {rebuild:?}" + ); + } + + /// Broadcaster whose first call (create-path) returns `MaybeSent` (leaving + /// the tracked row at `Built` with the funding reservation held) and whose + /// second call (resume-path) returns `Rejected` — the case where resume + /// pre-advances the row to `Broadcast` under the write lock and then the + /// broadcast definitively fails to reach the network. + struct MaybeSentThenRejectedBroadcaster { + call_count: AtomicUsize, + } + + #[async_trait] + impl TransactionBroadcaster for MaybeSentThenRejectedBroadcaster { + async fn broadcast(&self, _transaction: &Transaction) -> Result { + let call = self.call_count.fetch_add(1, Ordering::SeqCst); + if call == 0 { + Err(BroadcastError::MaybeSent { + reason: "create-path leaves the row at Built for a later resume".to_string(), + }) + } else { + Err(BroadcastError::Rejected { + reason: "resume-side rejection after the pre-advance to Broadcast".to_string(), + }) + } + } + } + + /// After the `Built`-arm race-guard advances the tracked row to + /// `Broadcast`, a resume-side broadcast that returns `Rejected` must keep + /// that status: the `Broadcast` arm can defensively re-broadcast on later + /// resumes, and rolling back could clobber a concurrent successful resume. + /// The funding reservation must stay held (the row is still tracked), and + /// no persisted-row deletion must be queued. + #[tokio::test] + async fn resume_side_rejected_after_pre_advance_keeps_row_at_broadcast() { + let broadcaster = Arc::new(MaybeSentThenRejectedBroadcaster { + call_count: AtomicUsize::new(0), + }); + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + + // 1. Create leaves the row at `Built` (MaybeSent keeps the reservation + // and the resumable row). + let create_result = manager + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + assert!( + matches!( + create_result, + Err(PlatformWalletError::TransactionBroadcastUnconfirmed(_)) + ), + "create-side MaybeSent should surface as TransactionBroadcastUnconfirmed, got {create_result:?}" + ); + let out_point = { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + assert_eq!(info.tracked_asset_locks.len(), 1); + let (op, lock) = info + .tracked_asset_locks + .iter() + .next() + .expect("built row tracked"); + assert_eq!(lock.status, AssetLockStatus::Built); + *op + }; + + // 2. Resume: pre-advances Built → Broadcast under the write lock, then + // calls `broadcast(&tx)` which returns `Rejected`. The `Rejected` + // surfaces to the caller. + let resume_result = manager + .resume_asset_lock(&out_point, Some(Duration::from_millis(10))) + .await; + assert!( + matches!( + resume_result, + Err(PlatformWalletError::TransactionBroadcast(_)) + ), + "resume-side Rejected should surface as TransactionBroadcast, got {resume_result:?}" + ); + assert_eq!( + broadcaster.call_count.load(Ordering::SeqCst), + 2, + "both create and resume must have attempted a broadcast" + ); + + // 3. Row remains at `Broadcast` — the pre-advance is retained because + // another resume may already own that shared status. + { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + assert_eq!( + info.tracked_asset_locks.len(), + 1, + "row must remain tracked for a later Broadcast-arm resume" + ); + let lock = info + .tracked_asset_locks + .get(&out_point) + .expect("row still tracked"); + assert_eq!( + lock.status, + AssetLockStatus::Broadcast, + "resume-side Rejected after pre-advance must keep Broadcast \ + so it does not clobber a concurrent successful resume, got {:?}", + lock.status + ); + } + + // 4. No persisted-row deletion was queued — the row must survive for + // a later resume. + assert!( + persistence.removed_outpoints().is_empty(), + "Broadcast row must not be queued for deletion, got {:?}", + persistence.removed_outpoints() + ); + + // 5. Reservation is still held — the row is tracked, so a fresh build + // over the single-UTXO wallet fails at input selection. + let rebuild = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + assert!( + matches!(rebuild, Err(PlatformWalletError::AssetLockTransaction(_))), + "rebuild must fail at input selection while the reservation is \ + held for the retained Broadcast row, got {rebuild:?}" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index 629d89b9eca..be02ffb4f90 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -4,7 +4,7 @@ //! resolving status from wallet info, resuming interrupted locks, //! and re-deriving private keys. -use crate::broadcaster::TransactionBroadcaster; +use crate::broadcaster::{BroadcastError, TransactionBroadcaster}; use std::time::Duration; use dashcore::Address as DashAddress; @@ -254,12 +254,37 @@ impl AssetLockManager { // 2. Resume from the current status. let proof = match status { AssetLockStatus::Built => { - // Re-broadcast and wait for proof. - self.broadcaster.broadcast(&tx).await?; + // Advance the tracked row to `Broadcast` BEFORE calling + // `broadcast(&tx)`. The snapshot above dropped the read lock, + // so a concurrent create-path Rejected cleanup can race the + // re-broadcast: if the row is still `Built` when + // `untrack_asset_lock` runs, the guard doesn't fire, the row + // is deleted, and the funding reservation is released while + // this call is still handing the same transaction to the + // network. Advancing first pushes the status past `Built` + // under the write lock, so either (a) we win and the untrack + // guard preserves the row + reservation, or (b) untrack ran + // first, the row is already gone, and this advance fails + // before we ever call `broadcast(&tx)`. let cs = self .advance_asset_lock_status(out_point, AssetLockStatus::Broadcast, None) .await?; self.queue_asset_lock_changeset(cs); + match self.broadcaster.broadcast(&tx).await { + Ok(_) => {} + Err(e @ BroadcastError::Rejected { .. }) => { + // Keep `Broadcast`: a concurrent successful resume + // may own that status, and the `Broadcast` arm + // defensively re-broadcasts on later resumes. + return Err(e.into()); + } + Err(e @ BroadcastError::MaybeSent { .. }) => { + // Outcome unknown — the tx may already be + // propagating. Keep `Broadcast` so a later resume + // can defensively re-broadcast and wait for proof. + return Err(e.into()); + } + } let proof = self.wait_for_proof(out_point, timeout).await?; self.validate_or_upgrade_proof(proof, account_index, out_point) .await? From cf4d33de00ac0ce426f7a0ef3a9f719bd1ebd1bb Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 25 Jul 2026 05:25:59 -0500 Subject: [PATCH 02/12] fix(platform-wallet): compare-and-set the Built -> Broadcast resume promotion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resume_asset_lock` snapshots the tracked row under a read lock and drops it before the pre-broadcast promotion, so two callers can both observe `Built`. If the first one broadcasts, obtains a proof and finalizes the row to `InstantSendLocked` / `ChainLocked` (step 3), the delayed second caller still ran the unconditional `Built -> Broadcast` write: `advance_asset_lock_status` assigns the status regardless of the current value, and passing `proof: None` leaves the existing proof attached. That downgrades a finalized row to the inconsistent `Broadcast + Some(proof)` state, and since changesets are last-write-wins it gets persisted too. A later resume then takes the `Broadcast` arm, ignores the attached proof, and waits for a proof it already holds — unbounded for the user-facing funding flows. The `Rejected` / `MaybeSent` return paths kept the regression rather than unwinding it. Add `promote_built_to_broadcast`, a compare-and-set under the wallet write lock: it only promotes while the row is still `Built`, and otherwise mutates nothing and hands back the row's current status and proof. `resume_asset_lock` re-dispatches from those values, so a stale caller takes the already-have-a-proof arm instead of overwriting it. An untracked outpoint still errors, preserving the existing guard against re-broadcasting a rejected row whose funding reservation was released. `advance_asset_lock_status` keeps its unconditional semantics (its other call sites are monotonic proof-attaching advances) with a doc note pointing racing callers at the new helper. Regression test drives the interleave deterministically via a two-sided test-only gate: the resume signals once it has taken its `Built` snapshot, the test finalizes the row to `ChainLocked` with a proof, then releases the resume. Verified to fail without the compare-and-set (the stale caller downgrades the row and re-broadcasts) and pass with it. Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 189 +++++++++++++++++- .../src/wallet/asset_lock/manager.rs | 29 +++ .../src/wallet/asset_lock/sync/recovery.rs | 82 ++++++-- .../src/wallet/asset_lock/sync/tracking.rs | 72 +++++++ 4 files changed, 354 insertions(+), 18 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 7c30b47c9ed..0ca79939a11 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -625,7 +625,7 @@ mod tests { funded_wallet_manager, AlwaysMaybeSentBroadcaster, AlwaysOkBroadcaster, AlwaysRejectedBroadcaster, WalletSigner, }; - use crate::wallet::asset_lock::manager::AssetLockManager; + use crate::wallet::asset_lock::manager::{AssetLockManager, ResumePrePromoteGate}; use crate::wallet::asset_lock::tracked::AssetLockStatus; use crate::wallet::persister::WalletPersister; use crate::wallet::platform_wallet::PlatformWalletInfo; @@ -1715,4 +1715,191 @@ mod tests { held for the retained Broadcast row, got {rebuild:?}" ); } + + /// `MaybeSent` on every call (leaving the tracked row at `Built` with + /// its funding reservation held), counting calls so a test can assert + /// that a stale resume did NOT re-broadcast. + struct CountingMaybeSentBroadcaster { + call_count: AtomicUsize, + } + + #[async_trait] + impl TransactionBroadcaster for CountingMaybeSentBroadcaster { + async fn broadcast(&self, _transaction: &Transaction) -> Result { + self.call_count.fetch_add(1, Ordering::SeqCst); + Err(BroadcastError::MaybeSent { + reason: "create-path leaves the row at Built for a later resume".to_string(), + }) + } + } + + /// Regression: a `resume_asset_lock` holding a STALE `Built` snapshot + /// must not downgrade a row that a concurrent flow already finalized. + /// + /// Two resumes can both snapshot `Built` under the read lock (which is + /// dropped before the pre-broadcast promotion). If the first one + /// broadcasts, obtains a proof and stores `ChainLocked` + proof, an + /// unconditional `Built -> Broadcast` write from the delayed second + /// caller would downgrade the finalized row to `Broadcast` while + /// leaving the proof attached — an inconsistent `Broadcast + + /// Some(proof)` state that also gets persisted (changesets are + /// last-write-wins). A later resume would then take the `Broadcast` + /// arm and wait for a proof the row already holds, potentially + /// forever. + /// + /// The compare-and-set in `promote_built_to_broadcast` makes the + /// stale caller observe the advanced row instead, re-dispatch from its + /// current status, and reuse the attached proof. + #[tokio::test] + async fn stale_built_resume_does_not_downgrade_a_concurrently_finalized_row() { + use dpp::identity::state_transition::asset_lock_proof::chain::ChainAssetLockProof; + + // A `MaybeSent` create leaves the row at `Built` with the funding + // reservation held — the state a resume picks up from. The + // broadcaster counts calls so the test can prove the stale resume + // never re-broadcast (it took the already-have-a-proof arm). + let broadcaster = Arc::new(CountingMaybeSentBroadcaster { + call_count: AtomicUsize::new(0), + }); + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + let _ = manager + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + let out_point = { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let (op, lock) = info + .tracked_asset_locks + .iter() + .next() + .expect("built row tracked"); + assert_eq!(lock.status, AssetLockStatus::Built); + *op + }; + + // 1. Install the pre-promote gate, then start a resume. It takes the + // read-locked `Built` snapshot and parks before the + // compare-and-set — this is the stale caller. + let arrived = Arc::new(Notify::new()); + let release = Arc::new(Notify::new()); + *manager + .resume_pre_promote_gate + .lock() + .expect("resume pre-promote gate mutex") = Some(ResumePrePromoteGate { + arrived: Arc::clone(&arrived), + release: Arc::clone(&release), + }); + + let manager_stale = Arc::clone(&manager); + let stale_resume = tokio::spawn(async move { + manager_stale + .resume_asset_lock(&out_point, Some(Duration::from_millis(10))) + .await + }); + + // Wait until the resume has actually taken its `Built` snapshot. + // Without this the finalize below could land first and the resume + // would read `ChainLocked` directly — never exercising the race. + arrived.notified().await; + + // 2. While it is parked, another flow finalizes the SAME row to + // `ChainLocked` with a proof attached — exactly what the winning + // resume's step 3 does. + let chain_proof = dpp::prelude::AssetLockProof::Chain(ChainAssetLockProof { + core_chain_locked_height: 1234, + out_point, + }); + let cs = manager + .advance_asset_lock_status( + &out_point, + AssetLockStatus::ChainLocked, + Some(chain_proof.clone()), + ) + .await + .expect("finalize the row"); + manager.queue_asset_lock_changeset(cs); + + // 3. Release the stale resume. Without the compare-and-set it would + // now write `Broadcast` over the finalized row. + release.notify_one(); + let resumed = stale_resume.await.expect("stale resume task joined"); + + // The stale caller re-dispatched into the already-have-a-proof arm: + // it neither re-broadcast nor waited for a proof. Both are + // observable — `wait_for_proof` would have returned `FinalityTimeout` + // against the 10ms deadline (no SPV record exists in this fixture), + // and the `Built`/`Broadcast` arms both broadcast before waiting. + // + // The resume still fails at its LAST step (step 4, credit-output + // path re-derivation): this fixture's funding-account address pool + // does not retain the peeked credit-output address, so + // `rederive_credit_output_path` cannot resolve it. That is a + // pre-existing fixture limitation unrelated to the race — reaching + // it at all proves the proof was reused, since a resume that waited + // would have failed earlier with `FinalityTimeout`. + assert!( + matches!( + resumed, + Err(PlatformWalletError::AssetLockTransaction(ref m)) + if m.contains("not found in funding account") + ), + "stale resume must reach credit-output re-derivation (proving it \ + reused the attached proof rather than waiting for a new one), \ + got {resumed:?}" + ); + assert_eq!( + broadcaster.call_count.load(Ordering::SeqCst), + 1, + "only the create-path broadcast may have happened: a stale resume \ + that re-dispatched from ChainLocked must not re-broadcast" + ); + + // The row is still finalized: status never regressed to `Broadcast` + // and the proof is intact. + { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let lock = info + .tracked_asset_locks + .get(&out_point) + .expect("row still tracked"); + assert_eq!( + lock.status, + AssetLockStatus::ChainLocked, + "stale Built snapshot must not downgrade the finalized row" + ); + assert_eq!( + lock.proof.as_ref(), + Some(&chain_proof), + "the concurrently-attached proof must survive" + ); + } + + // No persisted changeset carries the inconsistent + // `Broadcast + Some(proof)` pair. + let stored = persistence + .stored + .lock() + .expect("capturing persistence mutex"); + let inconsistent = stored + .iter() + .filter_map(|cs| cs.asset_locks.as_ref()) + .filter_map(|al| al.asset_locks.get(&out_point)) + .any(|entry| entry.status == AssetLockStatus::Broadcast && entry.proof.is_some()); + assert!( + !inconsistent, + "no changeset may persist a Broadcast row with a proof attached" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index b9c810b6d26..b86859647ba 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -19,6 +19,17 @@ use key_wallet_manager::WalletManager; /// Default fee rate in duffs per kilobyte for asset lock transactions. pub(super) const DEFAULT_FEE_PER_KB: u64 = 1000; +/// Test-only rendezvous for +/// [`AssetLockManager::resume_pre_promote_gate`]. `arrived` fires once a +/// resume has taken its read-locked status snapshot; the resume then +/// blocks on `release`. +#[cfg(test)] +#[derive(Clone)] +pub(super) struct ResumePrePromoteGate { + pub(super) arrived: Arc, + pub(super) release: Arc, +} + /// Manages the full asset lock lifecycle: build, broadcast, proof, and tracking. /// /// Shared across sub-wallets via `Arc` so that any sub-wallet @@ -83,6 +94,22 @@ pub struct AssetLockManager { /// yet have collected its pool snapshot. #[cfg(test)] pub(super) build_serial_gate: std::sync::atomic::AtomicUsize, + /// Test-only pause point inside + /// [`resume_asset_lock`](Self::resume_asset_lock), between the + /// read-locked status snapshot and the write-locked `Built` → + /// `Broadcast` compare-and-set. When set, a resume signals `arrived` + /// and then awaits `release` at that point, which lets a test hold a + /// resume on a stale `Built` snapshot while another flow finalizes + /// the same row to `InstantSendLocked` / `ChainLocked` — the exact + /// interleave the compare-and-set exists to survive. + /// + /// Both halves matter for determinism: `arrived` proves the resume + /// really did snapshot `Built` BEFORE the test finalized the row (so + /// the snapshot under test is genuinely stale), and `release` holds + /// it there until the finalize has landed. `None` (the default) + /// makes the hook a no-op. + #[cfg(test)] + pub(super) resume_pre_promote_gate: std::sync::Mutex>, } impl AssetLockManager { @@ -105,6 +132,8 @@ impl AssetLockManager { build_persist_serial: tokio::sync::Mutex::new(()), #[cfg(test)] build_serial_gate: std::sync::atomic::AtomicUsize::new(0), + #[cfg(test)] + resume_pre_promote_gate: std::sync::Mutex::new(None), } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index be02ffb4f90..dd9c0c602ca 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -18,6 +18,7 @@ use crate::error::PlatformWalletError; use super::super::manager::AssetLockManager; use super::super::tracked::{AssetLockStatus, TrackedAssetLock}; +use super::tracking::BuiltPromotion; // --------------------------------------------------------------------------- // Blocking accessor (for synchronous / evo-tool contexts) @@ -219,7 +220,7 @@ impl AssetLockManager { tracing::info!(outpoint = %out_point, ?timeout, "resume_asset_lock: entered"); // 1. Look up the tracked lock — snapshot the fields we need. - let (tx, status, existing_proof, account_index) = { + let (tx, mut status, mut existing_proof, account_index) = { let wm = self.wallet_manager.read().await; let info = wm .get_wallet_info(&self.wallet_id) @@ -251,25 +252,72 @@ impl AssetLockManager { ) }; + // Test-only pause between the read-locked snapshot above and the + // write-locked compare-and-set below, so a test can deterministically + // hold this resume on a stale `Built` snapshot while another flow + // finalizes the same row. No-op unless a test installed the gate. + #[cfg(test)] + { + let gate = self + .resume_pre_promote_gate + .lock() + .expect("resume pre-promote gate mutex") + .clone(); + if let Some(gate) = gate { + // Signal first: the snapshot above is taken, so whatever the + // test does next is guaranteed to race a stale `Built`. + gate.arrived.notify_one(); + gate.release.notified().await; + } + } + + // 1b. Promote `Built` → `Broadcast` BEFORE calling `broadcast(&tx)`. + // The snapshot above dropped the read lock, so a concurrent + // create-path Rejected cleanup can race the re-broadcast: if the row + // is still `Built` when `untrack_asset_lock` runs, the guard doesn't + // fire, the row is deleted, and the funding reservation is released + // while this call is still handing the same transaction to the + // network. Advancing first pushes the status past `Built` under the + // write lock, so either (a) we win and the untrack guard preserves + // the row + reservation, or (b) untrack ran first, the row is + // already gone, and this promotion fails before we ever call + // `broadcast(&tx)`. + // + // The promotion is a compare-and-set rather than an unconditional + // write because that same dropped read lock lets TWO resumes both + // snapshot `Built`. If the first one broadcasts, obtains a proof and + // finalizes the row to `InstantSendLocked` / `ChainLocked` (step 3), + // an unconditional write from this delayed second caller would + // downgrade the finalized row to `Broadcast` while leaving the proof + // attached, and persist that inconsistent state. Instead we re-read + // the row's current status and proof under the write lock and + // re-dispatch from there — the arms below then take the already-have- + // a-proof path instead of waiting again for a proof we already hold. + if status == AssetLockStatus::Built { + match self.promote_built_to_broadcast(out_point).await? { + BuiltPromotion::Promoted(cs) => self.queue_asset_lock_changeset(cs), + BuiltPromotion::AlreadyAdvanced { + current_status, + current_proof, + } => { + tracing::info!( + outpoint = %out_point, + status = ?current_status, + has_proof = current_proof.is_some(), + "resume_asset_lock: row advanced past Built concurrently — \ + re-dispatching from its current state" + ); + status = current_status; + existing_proof = current_proof; + } + } + } + // 2. Resume from the current status. let proof = match status { AssetLockStatus::Built => { - // Advance the tracked row to `Broadcast` BEFORE calling - // `broadcast(&tx)`. The snapshot above dropped the read lock, - // so a concurrent create-path Rejected cleanup can race the - // re-broadcast: if the row is still `Built` when - // `untrack_asset_lock` runs, the guard doesn't fire, the row - // is deleted, and the funding reservation is released while - // this call is still handing the same transaction to the - // network. Advancing first pushes the status past `Built` - // under the write lock, so either (a) we win and the untrack - // guard preserves the row + reservation, or (b) untrack ran - // first, the row is already gone, and this advance fails - // before we ever call `broadcast(&tx)`. - let cs = self - .advance_asset_lock_status(out_point, AssetLockStatus::Broadcast, None) - .await?; - self.queue_asset_lock_changeset(cs); + // Promoted to `Broadcast` in step 1b — this arm owns that + // promotion, so it is the one that re-broadcasts. match self.broadcaster.broadcast(&tx).await { Ok(_) => {} Err(e @ BroadcastError::Rejected { .. }) => { diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index fd101ddba32..8a3503e49e3 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -9,6 +9,22 @@ use crate::error::PlatformWalletError; use super::super::manager::AssetLockManager; use super::super::tracked::{AssetLockStatus, TrackedAssetLock}; +/// Outcome of the compare-and-set `Built` → `Broadcast` promotion in +/// [`AssetLockManager::promote_built_to_broadcast`]. +#[derive(Debug)] +pub(crate) enum BuiltPromotion { + /// The row was still `Built` and is now `Broadcast`. Carries the + /// changeset the caller must queue. + Promoted(AssetLockChangeSet), + /// A concurrent flow already advanced the row past `Built`. Nothing + /// was mutated; the caller re-dispatches from these values rather + /// than overwriting them. + AlreadyAdvanced { + current_status: AssetLockStatus, + current_proof: Option, + }, +} + impl AssetLockManager { /// The recorded [`AssetLockFundingType`] of a tracked lock, or `None` /// when the outpoint is not tracked. Used by the funding resolver to @@ -157,10 +173,66 @@ impl AssetLockManager { Ok(cs) } + /// Compare-and-set the pre-broadcast + /// [`Built`](AssetLockStatus::Built) → + /// [`Broadcast`](AssetLockStatus::Broadcast) promotion, under the + /// wallet write lock. + /// + /// `resume_asset_lock` snapshots the tracked row under a *read* lock + /// and drops it before promoting, so two callers can both observe + /// `Built`. If the first one goes on to broadcast, obtain a proof and + /// store `InstantSendLocked` / `ChainLocked` with that proof attached, + /// an unconditional write from the delayed second caller would + /// downgrade the finalized row back to `Broadcast` while leaving the + /// proof attached — an inconsistent `Broadcast + Some(proof)` state + /// that also gets persisted (changesets are last-write-wins). A later + /// resume would then take the `Broadcast` arm and wait for a proof it + /// already holds. + /// + /// So the promotion only fires while the row is *still* `Built`. + /// Otherwise nothing is mutated and the caller is handed the row's + /// current status and proof to re-dispatch from. + /// + /// Still errors when the outpoint is untracked: that means a + /// concurrent `untrack_asset_lock` removed a rejected row (releasing + /// its funding reservation), and the caller must abort before + /// re-broadcasting a transaction whose inputs are re-spendable. + pub(crate) async fn promote_built_to_broadcast( + &self, + out_point: &OutPoint, + ) -> Result { + let mut wm = self.wallet_manager.write().await; + let info = wm + .get_wallet_info_mut(&self.wallet_id) + .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; + let entry = info.tracked_asset_locks.get_mut(out_point).ok_or_else(|| { + PlatformWalletError::AssetLockProofWait(format!( + "Asset lock {} is not tracked", + out_point + )) + })?; + + if entry.status != AssetLockStatus::Built { + return Ok(BuiltPromotion::AlreadyAdvanced { + current_status: entry.status.clone(), + current_proof: entry.proof.clone(), + }); + } + + entry.status = AssetLockStatus::Broadcast; + let mut cs = AssetLockChangeSet::default(); + cs.asset_locks.insert(*out_point, (&*entry).into()); + Ok(BuiltPromotion::Promoted(cs)) + } + /// Advance the status of a tracked asset lock and optionally attach the proof. /// /// Returns an [`AssetLockChangeSet`] carrying a full snapshot of the /// updated entry. + /// + /// Assigns unconditionally — callers that race another writer for the + /// same row must gate the write themselves (see + /// [`promote_built_to_broadcast`](Self::promote_built_to_broadcast)). pub(crate) async fn advance_asset_lock_status( &self, out_point: &OutPoint, From d3345783d5fabf61f3bf090fb0715f576dd126a6 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 25 Jul 2026 05:43:58 -0500 Subject: [PATCH 03/12] fix(platform-wallet): compare-and-set the create-side Built -> Broadcast write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit closed the resume-side stale promotion, but the create path kept the same unconditional writer. `broadcast_funded_asset_lock` tracks the row as `Built` (step 2), then awaits `broadcaster.broadcast(&tx)` — an unbounded network call — before assigning `Broadcast` on the way out. A concurrent `resume_asset_lock` can pick the same outpoint up inside that window: both the FFI catch-up scanner (`asset_lock_manager_catch_up_blocking`) and the funding resolver's `FromExistingAssetLock` arm drive one for any tracked row. If it broadcasts, obtains a proof and finalizes the row to `InstantSendLocked` / `ChainLocked` (its step 3), the original call's `advance_asset_lock_status(.., Broadcast, None)` then downgrades that finalized row — and since `advance_asset_lock_status` leaves the existing proof attached when passed `None`, it recreates the inconsistent `Broadcast + Some(proof)` state and persists it (changesets are last-write-wins). A later resume takes the `Broadcast` arm, ignores the attached proof and waits for one it already holds — unbounded for the user-facing funding flows. Same end state the resume-side fix addressed, reached from the other writer. Route this call through `promote_built_to_broadcast` too, so the promotion only fires while the row is still `Built`. When it has advanced, its status and proof are strictly further along than anything this call could write, so they are left untouched and the successful broadcast is still reported as success. Unlike `resume_asset_lock` there is nothing to re-dispatch: this create-only half neither re-broadcasts nor waits, since the proof wait lives in `wait_for_funded_asset_lock_proof`, the caller's next step. `sync::tracking` becomes `pub(super)` so `build.rs` can name `BuiltPromotion`. With this, no unconditional `Built -> Broadcast` writer remains. The two promotion sites (create + resume) both compare-and-set; the four other `advance_asset_lock_status` call sites (create/resume proof attach, and the shielded / platform-address IS->CL upgrades) are all monotonic terminal writes carrying `Some(proof)` and never write `Broadcast`. Regression test drives the interleave deterministically: the broadcaster finalizes the row to `ChainLocked` with a proof in place — standing in for the winning resume — and then returns `Ok`. Verified red on the old implementation (row read back `Broadcast`, and independently the persisted `Broadcast + Some(proof)` changeset assertion fires) and green with the compare-and-set. The finalize is in-memory only, so any persisted inconsistent changeset could only have come from the create path itself. Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 214 +++++++++++++++++- .../src/wallet/asset_lock/sync/mod.rs | 6 +- .../src/wallet/asset_lock/sync/tracking.rs | 22 +- 3 files changed, 231 insertions(+), 11 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 0ca79939a11..48d28264861 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -20,6 +20,7 @@ use key_wallet::wallet::Wallet; use crate::error::PlatformWalletError; use super::manager::{AssetLockManager, DEFAULT_FEE_PER_KB}; +use super::sync::tracking::BuiltPromotion; use super::tracked::{AssetLockStatus, TrackedAssetLock}; // --------------------------------------------------------------------------- @@ -554,10 +555,44 @@ impl AssetLockManager { } // 4. Transition to Broadcast and queue the changeset. - let cs_broadcast = self - .advance_asset_lock_status(&out_point, AssetLockStatus::Broadcast, None) - .await?; - self.queue_asset_lock_changeset(cs_broadcast); + // + // Compare-and-set, not an unconditional write: the row was tracked as + // `Built` back in step 2 and the await above is unbounded, so a + // concurrent `resume_asset_lock` (the FFI catch-up scanner and the + // funding resolver both drive one for any tracked outpoint) can pick + // the row up, broadcast the same transaction, obtain a proof and + // finalize it to `InstantSendLocked` / `ChainLocked` while this call + // is still parked in `broadcast(&tx)`. Assigning `Broadcast` + // unconditionally on the way out would then downgrade that finalized + // row, and because `advance_asset_lock_status` leaves the existing + // proof attached when passed `None`, it would recreate — and persist, + // changesets being last-write-wins — the inconsistent + // `Broadcast + Some(proof)` state. A later resume takes the + // `Broadcast` arm and waits for a proof the row already holds, + // unbounded for the user-facing funding flows. + // + // So promote only while the row is still `Built`. If it advanced, its + // status and proof are strictly further along than anything this call + // could write, so leave them untouched: our broadcast still succeeded, + // which is all this create-only half reports. Unlike + // `resume_asset_lock` there is nothing to re-dispatch — the proof wait + // lives in `wait_for_funded_asset_lock_proof`, the caller's next step. + match self.promote_built_to_broadcast(&out_point).await? { + BuiltPromotion::Promoted(cs) => self.queue_asset_lock_changeset(cs), + BuiltPromotion::AlreadyAdvanced { + current_status, + current_proof, + } => { + tracing::info!( + outpoint = %out_point, + status = ?current_status, + has_proof = current_proof.is_some(), + "broadcast_funded_asset_lock: row advanced past Built \ + concurrently during the broadcast — keeping its current \ + state instead of downgrading it to Broadcast" + ); + } + } Ok((path, out_point)) } @@ -1902,4 +1937,175 @@ mod tests { "no changeset may persist a Broadcast row with a proof attached" ); } + + /// Broadcaster that stages the create-side counterpart of the race: + /// "during" the create path's own broadcast — i.e. while + /// `broadcast_funded_asset_lock` is parked in this await, after it + /// tracked the row as `Built` and before its `Built` → `Broadcast` + /// promotion — a concurrent `resume_asset_lock` picks the same outpoint + /// up, broadcasts, obtains a proof and finalizes the row to + /// `ChainLocked` (its step 3). Then the original broadcast returns `Ok`. + /// + /// Mutating the row in place stands in for that winning resume without + /// needing a second task: the point under test is what the create path + /// writes on its way out, and doing it inline makes the interleave + /// deterministic rather than scheduler-dependent. The finalize is + /// deliberately in-memory only, so any persisted + /// `Broadcast + Some(proof)` changeset can only have come from the + /// create path itself. + struct FinalizeDuringBroadcastBroadcaster { + wallet_manager: Arc>>, + wallet_id: WalletId, + call_count: AtomicUsize, + /// The proof the simulated resume attaches, so the test can assert + /// it survived byte-for-byte. + proof: Mutex>, + } + + #[async_trait] + impl TransactionBroadcaster for FinalizeDuringBroadcastBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + self.call_count.fetch_add(1, Ordering::SeqCst); + use dpp::identity::state_transition::asset_lock_proof::chain::ChainAssetLockProof; + + let mut wm = self.wallet_manager.write().await; + let info = wm + .get_wallet_info_mut(&self.wallet_id) + .expect("wallet present"); + let lock = info + .tracked_asset_locks + .values_mut() + .next() + .expect("Built row tracked before broadcast"); + assert_eq!( + lock.status, + AssetLockStatus::Built, + "create path must have tracked the row as Built before broadcasting" + ); + let proof = dpp::prelude::AssetLockProof::Chain(ChainAssetLockProof { + core_chain_locked_height: 4016, + out_point: lock.out_point, + }); + lock.status = AssetLockStatus::ChainLocked; + lock.proof = Some(proof.clone()); + *self.proof.lock().expect("staged proof mutex") = Some(proof); + drop(wm); + + Ok(transaction.txid()) + } + } + + /// Regression: the create path's `Built` → `Broadcast` promotion must be + /// a compare-and-set too, not just resume's. + /// + /// `broadcast_funded_asset_lock` tracks the row as `Built`, then awaits + /// `broadcaster.broadcast(&tx)` — an unbounded network call. A + /// concurrent `resume_asset_lock` (the FFI catch-up scanner and the + /// funding resolver both drive one for any tracked outpoint) can pick + /// the row up in that window, broadcast the same transaction, obtain a + /// proof and finalize it to `InstantSendLocked` / `ChainLocked`. When + /// the original broadcast then returns `Ok`, an unconditional + /// `advance_asset_lock_status(.., Broadcast, None)` downgrades that + /// finalized row — and because `None` leaves the existing proof + /// attached, it recreates the inconsistent `Broadcast + Some(proof)` + /// state and persists it (changesets are last-write-wins). A later + /// resume takes the `Broadcast` arm and waits for a proof the row + /// already holds, unbounded for the user-facing funding flows. + /// + /// With the compare-and-set the promotion is skipped, the finalized + /// status and proof survive, and the successful broadcast is still + /// reported as success — this create-only half has nothing to + /// re-dispatch, since the proof wait lives in + /// `wait_for_funded_asset_lock_proof`. + #[tokio::test] + async fn create_broadcast_does_not_downgrade_a_row_finalized_during_the_broadcast() { + let (wallet_manager, wallet_id, _balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + + let broadcaster = Arc::new(FinalizeDuringBroadcastBroadcaster { + wallet_manager: Arc::clone(&wallet_manager), + wallet_id, + call_count: AtomicUsize::new(0), + proof: Mutex::new(None), + }); + let persistence = Arc::new(CapturingPersistence::default()); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let manager = Arc::new(AssetLockManager::new( + sdk, + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(Notify::new()), + Arc::clone(&broadcaster), + WalletPersister::new( + wallet_id, + Arc::clone(&persistence) as Arc, + ), + )); + + // The broadcast itself succeeds, so the create half reports success + // even though it did not own the final status. + let (_path, out_point) = manager + .broadcast_funded_asset_lock( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await + .expect("a successful broadcast must still be reported as success"); + + assert_eq!( + broadcaster.call_count.load(Ordering::SeqCst), + 1, + "the create-only half must broadcast exactly once — it has no \ + re-dispatch path and must not re-broadcast after observing the \ + advanced row" + ); + + let staged_proof = broadcaster + .proof + .lock() + .expect("staged proof mutex") + .clone() + .expect("the simulated resume staged a proof"); + + // The finalized row survives the create path's exit. + { + let wm = wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet still present"); + let lock = info + .tracked_asset_locks + .get(&out_point) + .expect("row still tracked"); + assert_eq!( + lock.status, + AssetLockStatus::ChainLocked, + "a row finalized during the broadcast must not be downgraded \ + to Broadcast on the way out" + ); + assert_eq!( + lock.proof.as_ref(), + Some(&staged_proof), + "the concurrently-attached proof must survive" + ); + } + + // …and the inconsistent pair was never persisted either. + let stored = persistence + .stored + .lock() + .expect("capturing persistence mutex"); + let inconsistent = stored + .iter() + .filter_map(|cs| cs.asset_locks.as_ref()) + .filter_map(|al| al.asset_locks.get(&out_point)) + .any(|entry| entry.status == AssetLockStatus::Broadcast && entry.proof.is_some()); + assert!( + !inconsistent, + "no changeset may persist a Broadcast row with a proof attached" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/mod.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/mod.rs index 751ea12e0e3..81cbe1c500d 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/mod.rs @@ -6,4 +6,8 @@ mod proof; mod recovery; -mod tracking; +/// `pub(super)` so the create path in `build.rs` — the other +/// `Built` → `Broadcast` writer — can name +/// [`BuiltPromotion`](tracking::BuiltPromotion) and share the same +/// compare-and-set instead of writing the status unconditionally. +pub(super) mod tracking; diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index 8a3503e49e3..8ac68b15d49 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -178,11 +178,19 @@ impl AssetLockManager { /// [`Broadcast`](AssetLockStatus::Broadcast) promotion, under the /// wallet write lock. /// - /// `resume_asset_lock` snapshots the tracked row under a *read* lock - /// and drops it before promoting, so two callers can both observe - /// `Built`. If the first one goes on to broadcast, obtain a proof and - /// store `InstantSendLocked` / `ChainLocked` with that proof attached, - /// an unconditional write from the delayed second caller would + /// Shared by BOTH `Built` → `Broadcast` writers, because both hold a + /// stale view of the row across an unbounded `broadcast(&tx)` await: + /// + /// - `resume_asset_lock` snapshots the row under a *read* lock and + /// drops it before promoting, so two resumes can both observe + /// `Built`. + /// - `broadcast_funded_asset_lock` tracks the row as `Built`, then + /// awaits its own broadcast before promoting — during which a + /// concurrent resume can pick the same outpoint up. + /// + /// Either way, if the other flow goes on to broadcast, obtain a proof + /// and store `InstantSendLocked` / `ChainLocked` with that proof + /// attached, an unconditional write from the delayed caller would /// downgrade the finalized row back to `Broadcast` while leaving the /// proof attached — an inconsistent `Broadcast + Some(proof)` state /// that also gets persisted (changesets are last-write-wins). A later @@ -191,7 +199,9 @@ impl AssetLockManager { /// /// So the promotion only fires while the row is *still* `Built`. /// Otherwise nothing is mutated and the caller is handed the row's - /// current status and proof to re-dispatch from. + /// current status and proof. `resume_asset_lock` re-dispatches from + /// them; the create path has nothing to re-dispatch (its proof wait + /// lives in a separate method) and simply leaves them intact. /// /// Still errors when the outpoint is untracked: that means a /// concurrent `untrack_asset_lock` removed a rejected row (releasing From dceaa4da934035489aa49677e053586f7b4296b0 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 25 Jul 2026 06:12:58 -0500 Subject: [PATCH 04/12] fix(platform-wallet): serialize asset-lock status mutation with its persist enqueue The `Built` -> `Broadcast` compare-and-set fixed the in-memory half of the race but left a persistence-ordering window open. `promote_built_to_broadcast` mutated the row under `wallet_manager.write()`, returned a `Broadcast` changeset, and released that lock before the caller ran `queue_asset_lock_changeset`. On the multi-thread runtime another flow could take the wallet lock in that window, finalize the same row to `InstantSendLocked` / `ChainLocked` with a proof, and enqueue that snapshot first -- after which the delayed promoter enqueued its older `Broadcast + None` snapshot last. Nothing downstream repairs the inversion: `FFIPersister::store_round` serializes rounds by acquisition order (so it faithfully preserves the reversal), `AssetLockChangeSet::merge` is last-write-wins per outpoint, and Swift's `persistAssetLocks` upsert overwrites `statusRaw` / `proofBytes` unconditionally. Memory stayed finalized while the durable row regressed to `Broadcast` with no proof -- and because the load path treats `statusRaw < 2` as still-pending, a restart resumed a lock whose proof it already had. Both the create and resume promotion callers had the window. Add `AssetLockManager::status_persist_serial` and hold it across mutation -> enqueue in every asset-lock status writer, folding the enqueue into the mutators so no caller can reorder the pair: - `track_asset_lock` (insert / Built) - `untrack_asset_lock` (rejected-row removal) - `promote_built_to_broadcast` (both Built -> Broadcast callers) - `advance_asset_lock_status` (proof attachment, IS -> CL upgrades) - `consume_asset_lock` (terminal Consumed) - `recover_asset_lock_blocking` (blocking_lock, matching its blocking_write) The mutex is acquired before `wallet_manager` everywhere and never the reverse, and is not held across the unbounded `broadcast` / `wait_for_proof` awaits -- only the two adjacent steps need ordering. A dedicated mutex rather than extending the wallet write guard because the persister call is synchronous and reenters Swift via FFI on iOS. Call semantics and error behavior are unchanged; the mutators still return their changeset for inspection. Regression tests (both fail without the mutex, pass with it): - `promotion_cannot_enqueue_a_stale_snapshot_after_a_concurrent_finalize` parks a promoter in the post-CAS / pre-enqueue window via a new test-only gate, finalizes and enqueues the same row from another task, releases the promoter, then asserts the durable row. Pre-fix it observes `Broadcast` where memory holds `ChainLocked`. The new `durable_asset_lock` helper folds stored rounds using the real downstream semantics (arrival order, LWW merge, unconditional upsert, removal delete), so it models what a restart reads. - `resume_promotion_enqueues_under_the_shared_ordering_mutex` pins that the resume path routes through the same primitive, so the fix cannot regress to covering only the create caller. Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 327 +++++++++++++++++- .../src/wallet/asset_lock/manager.rs | 64 ++++ .../src/wallet/asset_lock/sync/recovery.rs | 20 +- .../src/wallet/asset_lock/sync/tracking.rs | 205 ++++++++--- .../fund_from_asset_lock.rs | 5 +- .../wallet/shielded/fund_from_asset_lock.rs | 5 +- 6 files changed, 566 insertions(+), 60 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 48d28264861..e869e2171dc 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -500,7 +500,9 @@ impl AssetLockManager { // 2. Track as Built and queue the changeset onto the persister // so a crash after broadcast leaves a row we can recover from. - let cs_built = self + // `track_asset_lock` queues the changeset itself, as one + // serialized unit with the in-memory insert. + let _cs_built = self .track_asset_lock(TrackedAssetLock { out_point, transaction: tx.clone(), @@ -512,7 +514,6 @@ impl AssetLockManager { proof: None, }) .await; - self.queue_asset_lock_changeset(cs_built); tracing::debug!( %txid, @@ -530,6 +531,8 @@ impl AssetLockManager { // reservation and the resumable row. if let Err(e) = self.broadcaster.broadcast(&tx).await { if matches!(e, crate::broadcaster::BroadcastError::Rejected { .. }) { + // `untrack_asset_lock` queues the changeset itself, as one + // serialized unit with the in-memory removal. let cs_untrack = self.untrack_asset_lock(&out_point).await; // Release only when the Built row was actually removed. If // the untrack guard fired instead — a concurrent @@ -538,9 +541,7 @@ impl AssetLockManager { // the inputs must stay reserved exactly like a `MaybeSent` // outcome, or the still-tracked row would be resumable while // its inputs are re-spendable. - let removed_built_row = cs_untrack.removed.contains(&out_point); - self.queue_asset_lock_changeset(cs_untrack); - if removed_built_row { + if cs_untrack.removed.contains(&out_point) { crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, &self.wallet_id, @@ -578,7 +579,10 @@ impl AssetLockManager { // `resume_asset_lock` there is nothing to re-dispatch — the proof wait // lives in `wait_for_funded_asset_lock_proof`, the caller's next step. match self.promote_built_to_broadcast(&out_point).await? { - BuiltPromotion::Promoted(cs) => self.queue_asset_lock_changeset(cs), + // The promotion queued its own changeset, atomically with the + // compare-and-set, so a concurrent finalize cannot have its + // newer snapshot overtaken by this older one. + BuiltPromotion::Promoted(_cs) => {} BuiltPromotion::AlreadyAdvanced { current_status, current_proof, @@ -628,10 +632,11 @@ impl AssetLockManager { dpp::prelude::AssetLockProof::Instant(_) => AssetLockStatus::InstantSendLocked, dpp::prelude::AssetLockProof::Chain(_) => AssetLockStatus::ChainLocked, }; - let cs_final = self + // Queued by `advance_asset_lock_status` itself, atomically with + // the in-memory write. + let _cs_final = self .advance_asset_lock_status(out_point, status, Some(proof.clone())) .await?; - self.queue_asset_lock_changeset(cs_final); Ok(proof) } @@ -654,13 +659,16 @@ mod tests { use crate::broadcaster::{BroadcastError, TransactionBroadcaster}; use crate::changeset::{ - ClientStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, + AssetLockEntry, ClientStartState, PersistenceError, PlatformWalletChangeSet, + PlatformWalletPersistence, }; use crate::test_support::{ funded_wallet_manager, AlwaysMaybeSentBroadcaster, AlwaysOkBroadcaster, AlwaysRejectedBroadcaster, WalletSigner, }; - use crate::wallet::asset_lock::manager::{AssetLockManager, ResumePrePromoteGate}; + use crate::wallet::asset_lock::manager::{ + AssetLockManager, PromotePostCasGate, ResumePrePromoteGate, + }; use crate::wallet::asset_lock::tracked::AssetLockStatus; use crate::wallet::persister::WalletPersister; use crate::wallet::platform_wallet::PlatformWalletInfo; @@ -679,6 +687,41 @@ mod tests { } impl CapturingPersistence { + /// The durable row for `out_point` after replaying every stored + /// round in arrival order. + /// + /// Models the real downstream semantics exactly, which is what + /// makes this a persistence-order assertion rather than a + /// "was it ever queued" one: + /// + /// - `FFIPersister::store_round` serializes rounds by acquisition + /// order, so replay order == the order `store()` was called; + /// - `AssetLockChangeSet::merge` is last-write-wins per outpoint; + /// - Swift's `persistAssetLocks` upsert overwrites `statusRaw` / + /// `proofBytes` unconditionally, and each `removed` entry + /// deletes the row. + /// + /// `None` means no row survives — either none was ever written or + /// the last round removed it. Since the load path reconstructs + /// `tracked_asset_locks` from exactly these rows, this is also + /// what a restart would read back. + fn durable_asset_lock(&self, out_point: &OutPoint) -> Option { + let stored = self.stored.lock().expect("capturing persistence mutex"); + let mut row: Option = None; + for cs in stored.iter() { + let Some(al) = cs.asset_locks.as_ref() else { + continue; + }; + if let Some(entry) = al.asset_locks.get(out_point) { + row = Some(entry.clone()); + } + if al.removed.contains(out_point) { + row = None; + } + } + row + } + /// Outpoints queued for persisted-row deletion across all stored /// changesets. fn removed_outpoints(&self) -> Vec { @@ -2108,4 +2151,268 @@ mod tests { "no changeset may persist a Broadcast row with a proof attached" ); } + + /// Drives a promoter into the post-CAS / pre-enqueue window, finalizes + /// and enqueues the same row from another task while it is parked, then + /// releases it — and returns the durable row that results. + /// + /// Shared by both `Built` → `Broadcast` regression tests below so the + /// create and resume paths are proven against the *same* interleave. + /// The promotion is invoked directly rather than through + /// `broadcast_funded_asset_lock` / `resume_asset_lock` because it is + /// the single primitive both callers now route through — see the + /// per-path tests for the proof that they do. + async fn durable_row_after_promote_finalize_interleave( + manager: Arc>, + persistence: Arc, + out_point: OutPoint, + ) -> (Option, dpp::prelude::AssetLockProof) { + use dpp::identity::state_transition::asset_lock_proof::chain::ChainAssetLockProof; + + // 1. Park a promoter between its compare-and-set and its enqueue — + // the exact window in which the stale `Broadcast + None` + // snapshot had not yet been handed to the persister. + let arrived = Arc::new(Notify::new()); + let release = Arc::new(Notify::new()); + *manager + .promote_post_cas_gate + .lock() + .expect("promote post-CAS gate mutex") = Some(PromotePostCasGate { + arrived: Arc::clone(&arrived), + release: Arc::clone(&release), + }); + + let manager_promoter = Arc::clone(&manager); + let promoter = tokio::spawn(async move { + manager_promoter + .promote_built_to_broadcast(&out_point) + .await + }); + + // The promoter has mutated the row to `Broadcast` in memory and is + // holding before the enqueue. Its snapshot is now genuinely stale + // with respect to anything written next. + arrived.notified().await; + + // 2. Finalize the SAME row to `ChainLocked + proof` and enqueue + // that — what a winning concurrent flow's step 3 does. Runs in + // its own task: with the fix this call BLOCKS on the ordering + // mutex until the promoter enqueues, so awaiting it inline here + // would deadlock against the `release` below. + let chain_proof = dpp::prelude::AssetLockProof::Chain(ChainAssetLockProof { + core_chain_locked_height: 4016, + out_point, + }); + let manager_finalizer = Arc::clone(&manager); + let finalize_proof = chain_proof.clone(); + let finalizer = tokio::spawn(async move { + manager_finalizer + .advance_asset_lock_status( + &out_point, + AssetLockStatus::ChainLocked, + Some(finalize_proof), + ) + .await + }); + + // Give the finalizer a real chance to get its enqueue in first. + // Pre-fix it does exactly that and the promoter's older snapshot + // lands last; post-fix it is queued behind the ordering mutex. + tokio::task::yield_now().await; + tokio::time::sleep(Duration::from_millis(50)).await; + + // 3. Release the stale promoter. + release.notify_one(); + promoter + .await + .expect("promoter task joined") + .expect("promotion must not error"); + finalizer + .await + .expect("finalizer task joined") + .expect("finalize must not error"); + + (persistence.durable_asset_lock(&out_point), chain_proof) + } + + /// Regression (persistence ordering): a `Built` → `Broadcast` promotion + /// must not let its older snapshot reach the persister AFTER a + /// concurrent finalize enqueued a newer proof-bearing one. + /// + /// The compare-and-set alone fixed only the in-memory half. The + /// promoter mutated the row under `wallet_manager.write()`, returned a + /// `Broadcast` changeset, and RELEASED that lock before the caller + /// enqueued it. In that post-CAS / pre-enqueue window another flow + /// could take the wallet lock, finalize the row to `ChainLocked + + /// proof`, and enqueue that snapshot first — after which the delayed + /// promoter enqueued its `Broadcast + None` one last. + /// + /// Nothing downstream repairs the inversion: `FFIPersister::store_round` + /// serializes rounds by acquisition order (preserving the reversal), + /// `AssetLockChangeSet::merge` is last-write-wins, and Swift's + /// `persistAssetLocks` upsert overwrites `statusRaw` / `proofBytes` + /// unconditionally. So memory stayed `ChainLocked` while the DURABLE row + /// regressed to `Broadcast` with no proof — and since the load path + /// treats `statusRaw < 2` as still-pending, a restart resumed a lock + /// whose proof it already had. + /// + /// Pre-fix this test observes the regressed durable row and fails; the + /// shared `status_persist_serial` makes mutation+enqueue one unit, so + /// the finalize now blocks until the promoter has enqueued and the + /// durable order matches the in-memory order. + #[tokio::test] + async fn promotion_cannot_enqueue_a_stale_snapshot_after_a_concurrent_finalize() { + // A `MaybeSent` create leaves the row `Built` — the state a + // promotion acts on. + let broadcaster = Arc::new(CountingMaybeSentBroadcaster { + call_count: AtomicUsize::new(0), + }); + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + let _ = manager + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + let out_point = { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let (op, lock) = info + .tracked_asset_locks + .iter() + .next() + .expect("built row tracked"); + assert_eq!(lock.status, AssetLockStatus::Built); + *op + }; + + let (durable, chain_proof) = durable_row_after_promote_finalize_interleave( + Arc::clone(&manager), + Arc::clone(&persistence), + out_point, + ) + .await; + + // In-memory finality was never in question — it is the durable + // state that regressed. + { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let lock = info + .tracked_asset_locks + .get(&out_point) + .expect("row still tracked"); + assert_eq!(lock.status, AssetLockStatus::ChainLocked); + } + + let durable = durable.expect("a durable row must exist"); + assert_eq!( + durable.status, + AssetLockStatus::ChainLocked, + "the durable row must not regress below the finalized in-memory \ + status: a stale promoter enqueued its Broadcast snapshot after \ + the finalize, and last-write-wins made it the row a restart reads" + ); + assert_eq!( + durable.proof.as_ref(), + Some(&chain_proof), + "the durable row must keep the finalized proof — dropping it makes \ + a restart re-wait for a proof the wallet already had" + ); + } + + /// The resume path routes its `Built` → `Broadcast` promotion through + /// the same serialized primitive as the create path. + /// + /// The test above proves the primitive orders mutation before enqueue; + /// this one pins that `resume_asset_lock` actually goes through it, so + /// the fix cannot regress to covering only the create caller. Asserted + /// structurally rather than by re-running the interleave: the resume + /// promotion happens mid-call, so parking it on the post-CAS gate would + /// stall the whole resume rather than isolate the window. + #[tokio::test] + async fn resume_promotion_enqueues_under_the_shared_ordering_mutex() { + let broadcaster = Arc::new(CountingMaybeSentBroadcaster { + call_count: AtomicUsize::new(0), + }); + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + let _ = manager + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + let out_point = { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + *info + .tracked_asset_locks + .keys() + .next() + .expect("built row tracked") + }; + + // Hold the ordering mutex, then start a resume. Its promotion must + // block on the mutex, which means it cannot have enqueued anything. + let serial = manager.status_persist_serial.lock().await; + let queued_before = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + + let manager_resume = Arc::clone(&manager); + let resume = tokio::spawn(async move { + manager_resume + .resume_asset_lock(&out_point, Some(Duration::from_millis(10))) + .await + }); + tokio::time::sleep(Duration::from_millis(50)).await; + + assert_eq!( + persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(), + queued_before, + "a resume whose promotion is blocked on `status_persist_serial` \ + must not have enqueued an asset-lock changeset — if it did, the \ + resume path is bypassing the shared ordering mutex and can still \ + reorder against a concurrent finalize" + ); + assert!( + manager.status_persist_serial.try_lock().is_err(), + "the ordering mutex must still be held by this test — otherwise the \ + assertion above proves nothing about serialization" + ); + + drop(serial); + let _ = resume.await.expect("resume task joined"); + + // Once released the promotion completes and its snapshot is durable. + let durable = persistence + .durable_asset_lock(&out_point) + .expect("the released resume must have enqueued its promotion"); + assert!( + durable.status != AssetLockStatus::Built, + "the resume's promotion must have advanced the durable row past \ + Built, got {:?}", + durable.status + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index b86859647ba..728a921b567 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -30,6 +30,22 @@ pub(super) struct ResumePrePromoteGate { pub(super) release: Arc, } +/// Test-only rendezvous for +/// [`AssetLockManager::promote_post_cas_gate`]. `arrived` fires once a +/// `Built` → `Broadcast` promotion has mutated the in-memory row but has +/// NOT yet enqueued its changeset; the promoter then blocks on `release`. +/// +/// This is precisely the window in which a stale promoter could enqueue +/// its older `Broadcast + None` snapshot AFTER a concurrent flow already +/// enqueued a newer proof-bearing one — the ordering hazard +/// [`AssetLockManager::status_persist_serial`] exists to close. +#[cfg(test)] +#[derive(Clone)] +pub(super) struct PromotePostCasGate { + pub(super) arrived: Arc, + pub(super) release: Arc, +} + /// Manages the full asset lock lifecycle: build, broadcast, proof, and tracking. /// /// Shared across sub-wallets via `Arc` so that any sub-wallet @@ -94,6 +110,42 @@ pub struct AssetLockManager { /// yet have collected its pool snapshot. #[cfg(test)] pub(super) build_serial_gate: std::sync::atomic::AtomicUsize, + /// Serializes every asset-lock **status mutation + persistence + /// enqueue** pair, so the order in which rows are mutated in memory + /// is the order in which their snapshots reach the persister. + /// + /// `wallet_manager`'s write lock alone is not enough. Each mutator + /// (`promote_built_to_broadcast`, `advance_asset_lock_status`, …) + /// takes it, mutates, and RELEASES it before returning the + /// changeset the caller then hands to `queue_asset_lock_changeset`. + /// In that post-mutation/pre-enqueue window another flow can acquire + /// the wallet lock, finalize the same row to `InstantSendLocked` / + /// `ChainLocked` with a proof, and enqueue that newer snapshot + /// first — after which the delayed writer enqueues its older + /// `Broadcast + None` snapshot LAST. Nothing downstream repairs the + /// inversion: `FFIPersister::store_round` only serializes rounds by + /// acquisition order (so it faithfully preserves the reversed + /// order), `AssetLockChangeSet::merge` is last-write-wins, and + /// Swift's `persistAssetLocks` upsert overwrites `statusRaw` / + /// `proofBytes` unconditionally. Memory stays finalized while the + /// durable row regresses to `Broadcast`, and because the load path + /// treats `statusRaw < 2` as still-pending, a restart resumes a lock + /// whose proof it already had. + /// + /// Held across mutation → enqueue, and deliberately NOT across the + /// unbounded awaits (`broadcast`, `wait_for_proof`) that surround + /// them: only the relative order of the two adjacent steps needs + /// serializing. A dedicated mutex rather than extending the + /// `wallet_manager` write guard because `queue_asset_lock_changeset` + /// calls the persister synchronously — on iOS that reenters Swift + /// via FFI — and holding the wallet lock across that would serialize + /// every unrelated wallet reader behind host I/O. + /// + /// Lock ordering: acquire this BEFORE `wallet_manager`, never the + /// reverse. Every holder follows the same + /// `status_persist_serial → wallet_manager.write() → drop(wallet) → + /// enqueue → drop(serial)` shape, so no cycle exists. + pub(super) status_persist_serial: tokio::sync::Mutex<()>, /// Test-only pause point inside /// [`resume_asset_lock`](Self::resume_asset_lock), between the /// read-locked status snapshot and the write-locked `Built` → @@ -110,6 +162,15 @@ pub struct AssetLockManager { /// makes the hook a no-op. #[cfg(test)] pub(super) resume_pre_promote_gate: std::sync::Mutex>, + /// Test-only pause point inside + /// [`promote_and_queue_built_to_broadcast`](Self::promote_and_queue_built_to_broadcast), + /// between the compare-and-set mutation and the persistence enqueue. + /// Lets a test hold a promoter in that window while another flow + /// finalizes and enqueues the same row, which is the interleave + /// [`Self::status_persist_serial`] exists to prevent. `None` (the + /// default) makes the hook a no-op. + #[cfg(test)] + pub(super) promote_post_cas_gate: std::sync::Mutex>, } impl AssetLockManager { @@ -130,10 +191,13 @@ impl AssetLockManager { broadcaster, persister, build_persist_serial: tokio::sync::Mutex::new(()), + status_persist_serial: tokio::sync::Mutex::new(()), #[cfg(test)] build_serial_gate: std::sync::atomic::AtomicUsize::new(0), #[cfg(test)] resume_pre_promote_gate: std::sync::Mutex::new(None), + #[cfg(test)] + promote_post_cas_gate: std::sync::Mutex::new(None), } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index dd9c0c602ca..c2350d1dda1 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -86,7 +86,16 @@ impl AssetLockManager { None => self.resolve_status_with_in_memory(in_memory_record, account_index, &out_point), }; - // Phase 3 (lock held): commit the tracked-asset-lock entry. + // Phase 3 (locks held): commit the tracked-asset-lock entry and + // enqueue it as one serialized unit. Without the ordering mutex + // a concurrent flow could finalize this row and enqueue its + // proof-bearing snapshot in the window between the insert below + // and the enqueue, leaving the older recovered snapshot durable + // (see `status_persist_serial`). `blocking_lock` matches the + // `blocking_write` already used here — this method is documented + // as callable only OUTSIDE a tokio async context. + let _serial = self.status_persist_serial.blocking_lock(); + // We re-check `tracked_asset_locks.contains_key` because // another caller could have raced in during phase 2 — first // writer wins. @@ -295,7 +304,9 @@ impl AssetLockManager { // a-proof path instead of waiting again for a proof we already hold. if status == AssetLockStatus::Built { match self.promote_built_to_broadcast(out_point).await? { - BuiltPromotion::Promoted(cs) => self.queue_asset_lock_changeset(cs), + // The promotion queued its own changeset, atomically with + // the compare-and-set — see `status_persist_serial`. + BuiltPromotion::Promoted(_cs) => {} BuiltPromotion::AlreadyAdvanced { current_status, current_proof, @@ -403,10 +414,11 @@ impl AssetLockManager { dpp::prelude::AssetLockProof::Instant(_) => AssetLockStatus::InstantSendLocked, dpp::prelude::AssetLockProof::Chain(_) => AssetLockStatus::ChainLocked, }; - let cs = self + // Queued by `advance_asset_lock_status` itself, atomically with + // the in-memory write. + let _cs = self .advance_asset_lock_status(out_point, new_status, Some(proof.clone())) .await?; - self.queue_asset_lock_changeset(cs); // 4. Re-derive the one-time credit-output derivation path. let path = { diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index 8ac68b15d49..cfe7b4dc57c 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -14,7 +14,9 @@ use super::super::tracked::{AssetLockStatus, TrackedAssetLock}; #[derive(Debug)] pub(crate) enum BuiltPromotion { /// The row was still `Built` and is now `Broadcast`. Carries the - /// changeset the caller must queue. + /// changeset, which has ALREADY been queued for persistence before + /// return — the value is surfaced only so tests and callers can + /// inspect the diff. Promoted(AssetLockChangeSet), /// A concurrent flow already advanced the row past `Built`. Nothing /// was mutated; the caller re-dispatches from these values rather @@ -45,17 +47,36 @@ impl AssetLockManager { } /// Track a new asset lock in memory, returning a changeset describing - /// the inserted entry. + /// the inserted entry. The changeset has ALREADY been queued for + /// persistence before return; the value is surfaced so callers and + /// tests can inspect the diff. /// /// If an entry already exists at `out_point`, it is overwritten. + /// + /// # Ordering + /// + /// Mutation and enqueue happen as one unit under + /// [`status_persist_serial`](AssetLockManager::status_persist_serial), + /// for the same reason the promotion does. The insert is what makes + /// the row visible to `resume_asset_lock`, so the moment the wallet + /// lock drops a resume can promote it to `Broadcast` and enqueue + /// that — and an unserialized `Built` enqueue landing afterwards + /// would regress the durable row to the pre-broadcast state. pub(crate) async fn track_asset_lock(&self, lock: TrackedAssetLock) -> AssetLockChangeSet { - let mut wm = self.wallet_manager.write().await; - let mut cs = AssetLockChangeSet::default(); - if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { - let out_point = lock.out_point; - cs.asset_locks.insert(out_point, (&lock).into()); - info.tracked_asset_locks.insert(out_point, lock); - } + let _serial = self.status_persist_serial.lock().await; + + let cs = { + let mut wm = self.wallet_manager.write().await; + let mut cs = AssetLockChangeSet::default(); + if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { + let out_point = lock.out_point; + cs.asset_locks.insert(out_point, (&lock).into()); + info.tracked_asset_locks.insert(out_point, lock); + } + cs + }; + + self.queue_asset_lock_changeset(cs.clone()); cs } @@ -75,25 +96,46 @@ impl AssetLockManager { /// [`Built`](AssetLockStatus::Built): if a concurrent flow advanced it /// (e.g. a `resume_asset_lock` that re-broadcast in the window between /// the rejected broadcast and this cleanup), the progress is kept - /// rather than clobbered. The caller queues the changeset (call sites - /// live in `asset_lock/build.rs`, inside the module). + /// rather than clobbered. + /// + /// The changeset has ALREADY been queued for persistence before + /// return; the value is surfaced so the caller can tell whether the + /// row was actually removed (`removed` non-empty) — `build.rs` gates + /// releasing the funding reservation on exactly that. + /// + /// # Ordering + /// + /// Mutation and enqueue happen as one unit under + /// [`status_persist_serial`](AssetLockManager::status_persist_serial). + /// The `Built` guard is itself a compare-and-set against the same + /// concurrent finalizer, so it needs the same protection: were the + /// row-deleting `removed` enqueue to land after a concurrent + /// finalize's proof-bearing snapshot, Swift would delete the row + /// that snapshot had just written. pub(crate) async fn untrack_asset_lock(&self, out_point: &OutPoint) -> AssetLockChangeSet { - let mut wm = self.wallet_manager.write().await; - let mut cs = AssetLockChangeSet::default(); - if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { - match info.tracked_asset_locks.get(out_point) { - Some(entry) if entry.status == AssetLockStatus::Built => { - info.tracked_asset_locks.remove(out_point); - cs.removed.insert(*out_point); + let _serial = self.status_persist_serial.lock().await; + + let cs = { + let mut wm = self.wallet_manager.write().await; + let mut cs = AssetLockChangeSet::default(); + if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { + match info.tracked_asset_locks.get(out_point) { + Some(entry) if entry.status == AssetLockStatus::Built => { + info.tracked_asset_locks.remove(out_point); + cs.removed.insert(*out_point); + } + Some(entry) => tracing::warn!( + outpoint = %out_point, + status = ?entry.status, + "untrack_asset_lock: lock advanced past Built concurrently — leaving it tracked" + ), + None => {} } - Some(entry) => tracing::warn!( - outpoint = %out_point, - status = ?entry.status, - "untrack_asset_lock: lock advanced past Built concurrently — leaving it tracked" - ), - None => {} } - } + cs + }; + + self.queue_asset_lock_changeset(cs.clone()); cs } @@ -147,6 +189,12 @@ impl AssetLockManager { &self, out_point: &OutPoint, ) -> Result { + // Hold the ordering mutex across mutate → enqueue so the + // terminal `Consumed` snapshot cannot be overtaken by a + // concurrent status writer's older one (see + // `status_persist_serial`). Acquired BEFORE `wallet_manager`. + let _serial = self.status_persist_serial.lock().await; + // Build the changeset under the write lock, then release the // lock before queueing — `queue_asset_lock_changeset` calls // the persister synchronously and we don't want to hold the @@ -207,9 +255,60 @@ impl AssetLockManager { /// concurrent `untrack_asset_lock` removed a rejected row (releasing /// its funding reservation), and the caller must abort before /// re-broadcasting a transaction whose inputs are re-spendable. + /// + /// # Ordering + /// + /// The compare-and-set and the persistence enqueue happen as one + /// unit under [`status_persist_serial`](AssetLockManager::status_persist_serial), + /// so a promotion can never enqueue its `Broadcast + None` snapshot + /// after a concurrent finalize enqueued a newer proof-bearing one. + /// Both `Built` → `Broadcast` callers (create and resume) go through + /// here, so neither can reorder against the other or against + /// `advance_asset_lock_status`. pub(crate) async fn promote_built_to_broadcast( &self, out_point: &OutPoint, + ) -> Result { + // Hold the ordering mutex across mutate → enqueue. Acquired + // BEFORE `wallet_manager` (see the field's lock-ordering note). + let _serial = self.status_persist_serial.lock().await; + + let promotion = self.compare_and_set_built_to_broadcast(out_point).await?; + + // Test-only pause in the post-CAS / pre-enqueue window. Under the + // serialization above a concurrent finalize now blocks here rather + // than slipping its newer snapshot in ahead of ours. + #[cfg(test)] + { + let gate = self + .promote_post_cas_gate + .lock() + .expect("promote post-CAS gate mutex") + .clone(); + if let Some(gate) = gate { + gate.arrived.notify_one(); + gate.release.notified().await; + } + } + + if let BuiltPromotion::Promoted(ref cs) = promotion { + self.queue_asset_lock_changeset(cs.clone()); + } + Ok(promotion) + } + + /// The compare-and-set half of + /// [`promote_built_to_broadcast`](Self::promote_built_to_broadcast): + /// mutates the in-memory row under the wallet write lock and returns + /// the resulting changeset WITHOUT queueing it. + /// + /// Private and callable only from `promote_built_to_broadcast`, which + /// owns the ordering mutex — splitting it out keeps the wallet write + /// guard scoped to the mutation so it is released before the + /// synchronous persister call. + async fn compare_and_set_built_to_broadcast( + &self, + out_point: &OutPoint, ) -> Result { let mut wm = self.wallet_manager.write().await; let info = wm @@ -238,34 +337,56 @@ impl AssetLockManager { /// Advance the status of a tracked asset lock and optionally attach the proof. /// /// Returns an [`AssetLockChangeSet`] carrying a full snapshot of the - /// updated entry. + /// updated entry. The changeset has ALREADY been queued for + /// persistence before return; the value is surfaced so callers and + /// tests can inspect the diff. /// /// Assigns unconditionally — callers that race another writer for the /// same row must gate the write themselves (see /// [`promote_built_to_broadcast`](Self::promote_built_to_broadcast)). + /// + /// # Ordering + /// + /// Like the promotion, the mutation and the enqueue happen as one + /// unit under [`status_persist_serial`](AssetLockManager::status_persist_serial). + /// This is the finalizing half of the pair: without the shared mutex + /// a proof-bearing `InstantSendLocked` / `ChainLocked` snapshot can + /// be enqueued BEFORE a concurrently-delayed promoter enqueues its + /// older `Broadcast + None` one, regressing the durable row. pub(crate) async fn advance_asset_lock_status( &self, out_point: &OutPoint, new_status: AssetLockStatus, proof: Option, ) -> Result { - let mut wm = self.wallet_manager.write().await; - let info = wm - .get_wallet_info_mut(&self.wallet_id) - .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; - let entry = info.tracked_asset_locks.get_mut(out_point).ok_or_else(|| { - PlatformWalletError::AssetLockProofWait(format!( - "Asset lock {} is not tracked", - out_point - )) - })?; - entry.status = new_status; - if proof.is_some() { - entry.proof = proof; - } + // Hold the ordering mutex across mutate → enqueue. Acquired + // BEFORE `wallet_manager` (see the field's lock-ordering note). + let _serial = self.status_persist_serial.lock().await; - let mut cs = AssetLockChangeSet::default(); - cs.asset_locks.insert(*out_point, (&*entry).into()); + // Scoped so the wallet write guard is released before the + // synchronous persister call below. + let cs = { + let mut wm = self.wallet_manager.write().await; + let info = wm + .get_wallet_info_mut(&self.wallet_id) + .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; + let entry = info.tracked_asset_locks.get_mut(out_point).ok_or_else(|| { + PlatformWalletError::AssetLockProofWait(format!( + "Asset lock {} is not tracked", + out_point + )) + })?; + entry.status = new_status; + if proof.is_some() { + entry.proof = proof; + } + + let mut cs = AssetLockChangeSet::default(); + cs.asset_locks.insert(*out_point, (&*entry).into()); + cs + }; + + self.queue_asset_lock_changeset(cs.clone()); Ok(cs) } } diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs index 0658ef74e56..7eb919202f3 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs @@ -238,7 +238,9 @@ impl PlatformAddressWallet { // the stale IS proof Platform just rejected. The // catch-up scanner / Resume path then has a // truthful status to work from. - let cs = self + // `advance_asset_lock_status` queues the changeset itself, + // atomically with the in-memory write. + let _cs = self .asset_locks .advance_asset_lock_status( &out_point, @@ -246,7 +248,6 @@ impl PlatformAddressWallet { Some(chain_proof.clone()), ) .await?; - self.asset_locks.queue_asset_lock_changeset(cs); submit_with_cl_height_retry(settings, |s| { addresses.top_up_with_signers( &self.sdk, diff --git a/packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs b/packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs index ff6c71fff39..97565f93c20 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs @@ -381,7 +381,9 @@ impl PlatformWallet { .asset_locks .upgrade_to_chain_lock_proof(&out_point, cl_wait) .await?; - let cs = self + // `advance_asset_lock_status` queues the changeset + // itself, atomically with the in-memory write. + let _cs = self .asset_locks .advance_asset_lock_status( &out_point, @@ -389,7 +391,6 @@ impl PlatformWallet { Some(chain_proof.clone()), ) .await?; - self.asset_locks.queue_asset_lock_changeset(cs); submit_with_cl_height_retry(settings, |s| { build_and_broadcast_shielded( sdk.clone(), From c3ca845a3081d24eb1df8b3778aa4224dd7e5be5 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 25 Jul 2026 06:45:16 -0500 Subject: [PATCH 05/12] test(platform-wallet): rendezvous the ordering tests on a real arrival signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two asset-lock concurrency tests used a 50 ms sleep as their only evidence that the competing task had reached the status-persistence ordering boundary. Elapsed time is not a rendezvous: - `durable_row_after_promote_finalize_interleave` released the parked promoter after the delay whether or not the finalizer had run. If the finalizer had not been scheduled yet, even the pre-fix implementation could enqueue in the non-regressing order and the test would falsely pass. - `resume_promotion_enqueues_under_the_shared_ordering_mutex` read "no new changeset after 50 ms" as proof the resume was blocked on the mutex, but an unscheduled resume produces the same silence — including if the resume path stopped taking the mutex altogether. Add a test-only waiter gauge on `status_persist_serial`, maintained by the new `lock_status_persist_serial` helper that every async acquirer now routes through. It is incremented before the `lock().await` and RAII-dropped on acquisition, so a non-zero count means a task is queued at the boundary and cannot get past it while the test holds the lock. Because the counter lives in the lock helper, an implementation that stops taking the mutex never increments it and the waiting test fails loudly instead of passing on silence. The interleave helper now waits for the finalizer to either enqueue or block on the mutex (mirroring the existing `build_serial_gate` test's regressed-or-serialized pattern), so the release happens only after the finalizer provably had its chance to go first. Test-only: production behavior is unchanged, and the gauge and helper compile out of non-test builds. Negative control: removing the five `status_persist_serial` acquisitions makes both tests fail 5/5 runs — the first on the durable-row ordering assertion (Broadcast vs ChainLocked), the second on the new rendezvous panic ("saw 0"), which is precisely the case the sleep version misread as success. Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 64 +++++++++-- .../src/wallet/asset_lock/manager.rs | 100 ++++++++++++++++++ .../src/wallet/asset_lock/sync/tracking.rs | 10 +- 3 files changed, 163 insertions(+), 11 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index e869e2171dc..5b3b8970297 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -2215,11 +2215,47 @@ mod tests { .await }); - // Give the finalizer a real chance to get its enqueue in first. - // Pre-fix it does exactly that and the promoter's older snapshot - // lands last; post-fix it is queued behind the ordering mutex. - tokio::task::yield_now().await; - tokio::time::sleep(Duration::from_millis(50)).await; + // Wait until the finalizer has provably had its chance to enqueue + // first — not merely until some time has passed. Exactly one of + // two states must be observed, mirroring the build-gate test + // above: + // + // - the finalizer ENQUEUED (a new changeset landed): the + // unserialized behavior, where nothing held it back and its + // newer snapshot is already durable. The promoter's older + // snapshot then lands last on release and the caller's ordering + // assertion fires — deterministically, because we release only + // after observing the store, never before it happened; + // - the finalizer is QUEUED on `status_persist_serial` (waiter + // gauge non-zero): the fixed behavior. The promoter holds that + // mutex across its parked window, so the finalizer came to rest + // at the boundary and provably cannot enqueue before we release. + // + // A sleep distinguished neither: "no new changeset yet" could just + // mean the finalizer had not been scheduled, so the pre-fix + // implementation could enqueue in the non-regressing order after + // the release and falsely pass. + let queued_before_finalize = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + loop { + let finalizer_enqueued = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len() + > queued_before_finalize; + let finalizer_blocked = manager + .status_serial_waiters + .load(std::sync::atomic::Ordering::SeqCst) + >= 1; + if finalizer_enqueued || finalizer_blocked { + break; + } + tokio::time::sleep(Duration::from_millis(5)).await; + } // 3. Release the stale promoter. release.notify_one(); @@ -2381,7 +2417,23 @@ mod tests { .resume_asset_lock(&out_point, Some(Duration::from_millis(10))) .await }); - tokio::time::sleep(Duration::from_millis(50)).await; + + // Rendezvous on the resume actually REACHING the ordering + // boundary, rather than sleeping and hoping it got there. The + // waiter gauge is incremented by the production lock helper + // before its `lock().await` and dropped on acquisition, so a + // count of 1 while this test holds the mutex means the resume's + // promotion is queued on it and cannot have enqueued anything. + // + // This is what makes the "nothing was queued" assertion below + // evidence of serialization. After a sleep it was not: an + // unchanged changeset count could equally mean the resume task + // had never been scheduled. It also fails loudly if the resume + // path ever stops routing its promotion through the mutex — + // the gauge would stay at zero and the wait would panic, where + // the sleep version would have read the resulting silence as + // success. + manager.await_status_serial_waiters(1).await; assert_eq!( persistence diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index 728a921b567..4582a2464dc 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -146,6 +146,31 @@ pub struct AssetLockManager { /// `status_persist_serial → wallet_manager.write() → drop(wallet) → /// enqueue → drop(serial)` shape, so no cycle exists. pub(super) status_persist_serial: tokio::sync::Mutex<()>, + /// Test-only gauge of tasks currently BLOCKED on + /// [`status_persist_serial`](Self::status_persist_serial): + /// incremented before the `lock().await` and decremented the moment + /// it is acquired (see + /// [`lock_status_persist_serial`](Self::lock_status_persist_serial)), + /// so a non-zero value means "some task reached the ordering + /// boundary and cannot get past it while the current holder keeps + /// the lock". + /// + /// This is the arrival signal the ordering tests rendezvous on. A + /// sleep only shows that time passed — it cannot distinguish "the + /// competing task is queued at the mutex" from "the competing task + /// has not been scheduled yet", so a test that released its parked + /// holder after a delay could grade an unserialized implementation + /// as passing whenever the scheduler happened to run things in the + /// non-regressing order. Because the counter is maintained by the + /// lock helper itself, an implementation that stops taking the + /// mutex never increments it, and the waiting test fails instead of + /// silently passing. + /// + /// Covers the async acquirers only. The one `blocking_lock` caller + /// (`recover_tracked_asset_lock`) runs outside the async runtime, + /// so no test can observe it mid-wait. + #[cfg(test)] + pub(super) status_serial_waiters: std::sync::atomic::AtomicUsize, /// Test-only pause point inside /// [`resume_asset_lock`](Self::resume_asset_lock), between the /// read-locked status snapshot and the write-locked `Built` → @@ -193,6 +218,8 @@ impl AssetLockManager { build_persist_serial: tokio::sync::Mutex::new(()), status_persist_serial: tokio::sync::Mutex::new(()), #[cfg(test)] + status_serial_waiters: std::sync::atomic::AtomicUsize::new(0), + #[cfg(test)] build_serial_gate: std::sync::atomic::AtomicUsize::new(0), #[cfg(test)] resume_pre_promote_gate: std::sync::Mutex::new(None), @@ -201,6 +228,79 @@ impl AssetLockManager { } } + /// Acquire [`status_persist_serial`](Self::status_persist_serial). + /// + /// Every async mutate→enqueue pair goes through here rather than + /// locking the field directly, so the test-only + /// [`status_serial_waiters`](Self::status_serial_waiters) gauge sees + /// every arrival at the ordering boundary. In non-test builds this + /// compiles to the bare `lock().await`. + pub(super) async fn lock_status_persist_serial(&self) -> tokio::sync::MutexGuard<'_, ()> { + // RAII rather than a bare decrement after the await: if the + // caller's future is dropped while still queued, the count must + // come back down, or a cancelled task would leave the gauge + // permanently non-zero and every later wait would return + // instantly on a phantom arrival. + #[cfg(test)] + struct WaiterGauge<'a>(&'a std::sync::atomic::AtomicUsize); + #[cfg(test)] + impl Drop for WaiterGauge<'_> { + fn drop(&mut self) { + self.0.fetch_sub(1, std::sync::atomic::Ordering::SeqCst); + } + } + #[cfg(test)] + let waiting = { + self.status_serial_waiters + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + WaiterGauge(&self.status_serial_waiters) + }; + + let guard = self.status_persist_serial.lock().await; + + // Dropped on acquisition, not on release: the gauge answers + // "who is still queued at the boundary", so the holder must not + // count itself. + #[cfg(test)] + drop(waiting); + + guard + } + + /// Test-only: wait until at least `n` tasks are blocked on + /// [`status_persist_serial`](Self::status_persist_serial). + /// + /// The rendezvous the ordering tests use in place of a sleep. The + /// caller must already hold the mutex (or otherwise know it is + /// held), so an arrival observed here is an arrival that provably + /// cannot proceed past the boundary. Polls rather than using a + /// `Notify` because the waiters are inside the production lock + /// helper, which must stay free of test-only signalling on the + /// contended path. + /// + /// Panics after ~10s so a genuine hang fails loudly instead of + /// running until the harness times out. + #[cfg(test)] + pub(super) async fn await_status_serial_waiters(&self, n: usize) { + for _ in 0..2_000 { + if self + .status_serial_waiters + .load(std::sync::atomic::Ordering::SeqCst) + >= n + { + return; + } + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + } + panic!( + "timed out waiting for {n} task(s) to block on status_persist_serial \ + (saw {}) — either the competing task never reached the ordering \ + boundary, or the code under test no longer acquires the mutex there", + self.status_serial_waiters + .load(std::sync::atomic::Ordering::SeqCst) + ); + } + /// Queue an `AssetLockChangeSet` onto the per-wallet persister. /// No-op when the changeset is empty. /// diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index cfe7b4dc57c..200e7b6ee03 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -63,7 +63,7 @@ impl AssetLockManager { /// that — and an unserialized `Built` enqueue landing afterwards /// would regress the durable row to the pre-broadcast state. pub(crate) async fn track_asset_lock(&self, lock: TrackedAssetLock) -> AssetLockChangeSet { - let _serial = self.status_persist_serial.lock().await; + let _serial = self.lock_status_persist_serial().await; let cs = { let mut wm = self.wallet_manager.write().await; @@ -113,7 +113,7 @@ impl AssetLockManager { /// finalize's proof-bearing snapshot, Swift would delete the row /// that snapshot had just written. pub(crate) async fn untrack_asset_lock(&self, out_point: &OutPoint) -> AssetLockChangeSet { - let _serial = self.status_persist_serial.lock().await; + let _serial = self.lock_status_persist_serial().await; let cs = { let mut wm = self.wallet_manager.write().await; @@ -193,7 +193,7 @@ impl AssetLockManager { // terminal `Consumed` snapshot cannot be overtaken by a // concurrent status writer's older one (see // `status_persist_serial`). Acquired BEFORE `wallet_manager`. - let _serial = self.status_persist_serial.lock().await; + let _serial = self.lock_status_persist_serial().await; // Build the changeset under the write lock, then release the // lock before queueing — `queue_asset_lock_changeset` calls @@ -271,7 +271,7 @@ impl AssetLockManager { ) -> Result { // Hold the ordering mutex across mutate → enqueue. Acquired // BEFORE `wallet_manager` (see the field's lock-ordering note). - let _serial = self.status_persist_serial.lock().await; + let _serial = self.lock_status_persist_serial().await; let promotion = self.compare_and_set_built_to_broadcast(out_point).await?; @@ -361,7 +361,7 @@ impl AssetLockManager { ) -> Result { // Hold the ordering mutex across mutate → enqueue. Acquired // BEFORE `wallet_manager` (see the field's lock-ordering note). - let _serial = self.status_persist_serial.lock().await; + let _serial = self.lock_status_persist_serial().await; // Scoped so the wallet write guard is released before the // synchronous persister call below. From f93f011925777b8f01c08cbbddd62669af87d2ff Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 25 Jul 2026 07:45:35 -0500 Subject: [PATCH 06/12] fix(platform-wallet): retire an asset-lock manager when its wallet is removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PlatformWallet` hands out `Arc>` clones and the FFI parks them in its own handle storage, so a handle can outlive `PlatformWalletManager::remove_wallet`. Wallet ids are deterministic in (seed, network), so re-importing the same mnemonic recreates the very same id over a fresh `PlatformWalletInfo` and a fresh `AssetLockManager` with its own `status_persist_serial`. Because the manager resolves everything through the shared `WalletManager` by `wallet_id` alone, the retained handle silently re-attached to the replacement wallet — and old and new managers then mutated and persisted the same asset-lock rows under *different* mutexes, reintroducing across instances exactly the stale-snapshot enqueue reversal serialization closes within one. The `untrack` arm is the sharpest: its changeset's `removed` set DELETES the durable row, so a stale handle could erase a lock the replacement wallet had just tracked. Give each manager an `active` flag and flip it in `deactivate()` while HOLDING that manager's `status_persist_serial`. That is what makes the retirement a barrier rather than a hint, in both directions: - it cannot take the mutex until any in-flight mutate→enqueue unit has released it, so removal never interrupts one halfway (row mutated in memory, changeset never handed to the persister); - every such unit re-reads the flag once it holds that same mutex, so an operation that started before the removal and was parked on an unbounded `broadcast` / proof wait fails before it can touch a wallet row or the persister. `track_asset_lock`, `untrack_asset_lock`, `consume_asset_lock`, `promote_built_to_broadcast`, `advance_asset_lock_status` and the blocking `recover_asset_lock_blocking` all take the check under the mutex, before their wallet lookup. The two tracking primitives now return `Result`; in `build.rs`'s rejected-broadcast path a refusal is treated like the existing untrack guard — the funding reservation is NOT released, since the wallet that took it is gone and `wallet_id` now resolves to a replacement that never reserved those inputs. Public entry points (`resume_asset_lock`, `broadcast_funded_asset_lock`, the top of `recover_asset_lock_blocking`) also pre-check, but only as an early-out; those checks are documented as non-authoritative because a removal can land during the very next await. `remove_wallet` resolves the `PlatformWallet` first and deactivates with no other lock held, then drops the shared `WalletManager` entry. Order matters: re-registration must `insert_wallet` into that shared manager, which still holds this wallet's entry at that point, so no replacement can exist until deactivation is done. Deactivating under `wallet_manager` would invert the documented `status_persist_serial -> wallet_manager` order and deadlock. `WalletNotFound` idempotency (which the FFI maps to ok) is unchanged. Two regression tests, both rendezvousing on the production `status_serial_waiters` gauge rather than on elapsed time: - `deactivation_waits_for_the_in_flight_unit_then_refuses_every_later_mutation` parks a promoter post-CAS, proves `deactivate` comes to rest at the ordering boundary while the flag is still set, proves the in-flight unit completes and reaches the persister, then proves every later primitive is refused and enqueues nothing. - `retained_asset_lock_manager_cannot_touch_a_reimported_wallet` keeps a handle across removal, re-imports the same mnemonic, asserts the id collides and the manager is a different instance, then proves the retained handle can neither mutate the replacement's in-memory row nor enqueue a single round through the shared persister. Includes a negative control that the same operations succeed through that exact handle while the wallet is live. Negative controls: dropping the `deactivate` call from `remove_wallet` fails the second test on the first stale `track_asset_lock` (it returns `Ok` against the replacement wallet); removing the `lock_status_persist_serial().await` from `deactivate` fails the first test at the rendezvous ("saw 0") — the bare-flag design the barrier replaces. Scope is asset-lock lifecycle only: no FFI handle redesign, and the separate non-blocking ChainLocked -> InstantSendLocked monotonicity finding is untouched. Co-Authored-By: Claude --- packages/rs-platform-wallet/src/error.rs | 23 ++ .../src/manager/wallet_lifecycle.rs | 388 ++++++++++++++++++ .../src/wallet/asset_lock/build.rs | 239 ++++++++++- .../src/wallet/asset_lock/manager.rs | 131 ++++++ .../src/wallet/asset_lock/sync/recovery.rs | 49 +++ .../src/wallet/asset_lock/sync/tracking.rs | 65 ++- 6 files changed, 887 insertions(+), 8 deletions(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 8d71c044aac..55fe5f1fc5f 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -101,6 +101,29 @@ pub enum PlatformWalletError { #[error("Asset lock proof waiting failed: {0}")] AssetLockProofWait(String), + /// The operation was issued through an `AssetLockManager` whose wallet + /// has since been removed from the `PlatformWalletManager`. + /// + /// Wallet ids are deterministic in (seed, network), so re-importing the + /// same mnemonic re-creates the very same id against a *fresh* + /// `PlatformWalletInfo` and a *fresh* `AssetLockManager`. A handle + /// retained across the removal (an FFI `asset_lock_manager` handle the + /// host never destroyed, or an in-flight resume task) resolves through + /// the shared `WalletManager` by id alone, so without this guard it + /// would silently start mutating and persisting the replacement + /// wallet's rows under a different `status_persist_serial` than the + /// live manager — reintroducing the very stale-snapshot reversal the + /// ordering mutex closes within one instance. + /// + /// Always a stale-handle bug on the caller's side; the fix is to + /// re-acquire the manager from the current `PlatformWallet`. + #[error( + "Asset lock manager for wallet {0} is no longer active — its wallet was \ + removed from the manager; re-acquire the asset lock manager from the \ + current wallet handle" + )] + AssetLockManagerInactive(String), + #[error("SDK error: {0}")] Sdk(#[from] dash_sdk::Error), diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 947121519be..e43b8a7b3b9 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -567,10 +567,62 @@ impl PlatformWalletManager

{ } /// Remove a wallet from the manager. + /// + /// # Asset-lock manager lifecycle + /// + /// The wallet's [`AssetLockManager`](crate::AssetLockManager) is + /// retired *before* the shared `WalletManager` entry is dropped, and + /// only then is anything else torn down. That ordering is + /// load-bearing, not tidiness: + /// + /// * Subordinate handles outlive this call. `PlatformWallet` hands out + /// `Arc>` clones (the FFI parks them in its own + /// handle storage), so a caller can still hold — and drive — the old + /// manager after the wallet is gone. The manager resolves state + /// through the shared `WalletManager` by `wallet_id` alone. + /// * `wallet_id` is deterministic in (seed, network). Re-importing the + /// same mnemonic recreates the *same* id over a brand-new + /// `PlatformWalletInfo` and a brand-new `AssetLockManager` with its + /// own `status_persist_serial`. A retained old manager would then be + /// live against replacement state, and the two managers would mutate + /// and persist the same asset-lock rows under *different* mutexes — + /// reintroducing across instances exactly the stale-snapshot enqueue + /// reversal that serialization fixes within one instance. + /// + /// `AssetLockManager::deactivate` closes that window from both sides: + /// it takes the old manager's `status_persist_serial`, so it cannot + /// land in the middle of somebody's mutate→enqueue unit (removal waits + /// for the unit to finish), and every such unit re-reads the flag once + /// it holds that same mutex, so anything parked on an await when the + /// flag flipped fails before it can touch a wallet row or the + /// persister. Because re-registration must first `insert_wallet` into + /// the shared `WalletManager` — which still holds this wallet's entry + /// at this point — no replacement can exist until deactivation is + /// already done. + /// + /// `deactivate` is called with no other lock held. It acquires + /// `status_persist_serial`, and holders of that mutex go on to take + /// `wallet_manager.write()`; calling it under `wallet_manager` would + /// invert the documented lock order and deadlock. + /// + /// Idempotency is unchanged: a wallet absent from `self.wallets` still + /// returns [`PlatformWalletError::WalletNotFound`] after the shared + /// `WalletManager` entry is cleaned up, and `deactivate` is itself a + /// no-op on an already-retired manager. pub async fn remove_wallet( &self, wallet_id: &WalletId, ) -> Result, PlatformWalletError> { + // Retire the asset-lock manager first, holding nothing else. Note + // the read guard is dropped before `deactivate` awaits. + let existing = { + let wallets = self.wallets.read().await; + wallets.get(wallet_id).map(Arc::clone) + }; + if let Some(wallet) = &existing { + wallet.asset_locks().deactivate().await; + } + let owned_identity_ids: Vec = { let mut wm = self.wallet_manager.write().await; let ids = match wm.get_wallet_info(wallet_id) { @@ -844,3 +896,339 @@ mod register_wallet_duplicate_tests { ); } } + +/// Cross-instance lifecycle: an `AssetLockManager` handle retained across +/// `remove_wallet` must not become live against the replacement wallet a +/// same-mnemonic re-import installs under the same deterministic id. +#[cfg(test)] +mod retained_asset_lock_manager_tests { + use std::sync::{Arc, Mutex}; + + use dashcore::OutPoint; + use key_wallet::mnemonic::{Language, Mnemonic}; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::wallet::managed_wallet_info::asset_lock_builder::AssetLockFundingType; + use key_wallet::Network; + + use crate::changeset::{ + AssetLockEntry, ClientStartState, PersistenceError, PlatformWalletChangeSet, + PlatformWalletPersistence, + }; + use crate::error::PlatformWalletError; + use crate::events::{EventHandler, PlatformEventHandler}; + use crate::wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; + use crate::wallet::platform_wallet::WalletId; + use crate::PlatformWalletManager; + + // Canonical all-`abandon` BIP-39 test vector. Deterministic, which is + // the whole point here: re-importing it yields the SAME wallet id. + const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ + abandon abandon abandon abandon abandon about"; + + /// Records every changeset so the test can assert what a retired + /// handle did — and did not — push into the SHARED persistence + /// pipeline the replacement wallet also writes through. + #[derive(Default)] + struct CapturingPersister { + stored: Mutex>, + } + + impl CapturingPersister { + /// The durable asset-lock row for `out_point` after replaying + /// every stored round in arrival order (last-write-wins per + /// outpoint; a `removed` entry deletes it). Same replay model as + /// the asset-lock module's own persistence-order tests. + fn durable_asset_lock(&self, out_point: &OutPoint) -> Option { + let stored = self.stored.lock().expect("capturing persister mutex"); + let mut row: Option = None; + for cs in stored.iter() { + let Some(al) = cs.asset_locks.as_ref() else { + continue; + }; + if let Some(entry) = al.asset_locks.get(out_point) { + row = Some(entry.clone()); + } + if al.removed.contains(out_point) { + row = None; + } + } + row + } + + /// Count of rounds carrying an asset-lock sub-changeset. Wallet + /// registration queues plenty of other rounds, so the lifecycle + /// assertions filter on this rather than the total. + fn asset_lock_rounds(&self) -> usize { + self.stored + .lock() + .expect("capturing persister mutex") + .iter() + .filter(|cs| cs.asset_locks.is_some()) + .count() + } + } + + impl PlatformWalletPersistence for CapturingPersister { + fn store( + &self, + _wallet_id: WalletId, + changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + self.stored + .lock() + .expect("capturing persister mutex") + .push(changeset); + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + Ok(ClientStartState::default()) + } + } + + struct NoopEventHandler; + impl EventHandler for NoopEventHandler {} + impl PlatformEventHandler for NoopEventHandler {} + + /// A synthetic tracked lock. The lifecycle assertions are about which + /// manager may mutate the row, not about how it was funded, so the + /// transaction can be an empty one — nothing here broadcasts. + fn tracked_lock(out_point: OutPoint, status: AssetLockStatus) -> TrackedAssetLock { + TrackedAssetLock { + out_point, + transaction: dashcore::Transaction { + version: 3, + lock_time: 0, + input: vec![], + output: vec![], + special_transaction_payload: None, + }, + account_index: 0, + funding_type: AssetLockFundingType::IdentityRegistration, + identity_index: 0, + amount: 1_000_000, + status, + proof: None, + } + } + + /// Regression: `remove_wallet` must retire the wallet's + /// `AssetLockManager`, so a handle retained across removal cannot + /// mutate or persist the state of the wallet a re-import installs + /// under the same id. + /// + /// `wallet_id` is deterministic in (seed, network) and the FFI parks + /// `Arc>` clones in its own handle storage, which + /// outlive `platform_wallet_manager_remove_wallet`. The manager + /// resolves everything through the shared `WalletManager` by + /// `wallet_id` alone, so before this fix the old handle silently + /// re-attached to the replacement `PlatformWalletInfo` — and the two + /// managers then mutated and persisted the same rows under *different* + /// `status_persist_serial` mutexes, reintroducing across instances the + /// snapshot reordering serialization fixes within one. + /// + /// The `untrack` arm is the sharpest: its changeset's `removed` set + /// DELETES the durable row, so a stale handle taking that path would + /// erase an asset lock the replacement wallet had just tracked. + #[tokio::test] + async fn retained_asset_lock_manager_cannot_touch_a_reimported_wallet() { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let persister = Arc::new(CapturingPersister::default()); + let event_handler: Arc = Arc::new(NoopEventHandler); + let manager = Arc::new(PlatformWalletManager::new( + sdk, + Arc::clone(&persister), + event_handler, + )); + + let network = Network::Testnet; + let mnemonic = + Mnemonic::from_phrase(TEST_MNEMONIC, Language::English).expect("valid test mnemonic"); + let seed_bytes = mnemonic.to_seed(""); + + // `Some(0)` skips the SPV birth-height lookup, so nothing here + // consults SPV or the network. + let original = manager + .create_wallet_from_seed_bytes( + network, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("first create should succeed"); + let wallet_id = original.wallet_id(); + let retained = Arc::clone(original.asset_locks()); + + // Negative control: while the wallet is registered, the very + // operations asserted-refused below succeed through this exact + // handle. Without it, "the retained handle failed" could just + // mean the test never had a working mutation path. + let control_out_point = OutPoint::null(); + retained + .track_asset_lock(tracked_lock(control_out_point, AssetLockStatus::Built)) + .await + .expect("a live manager must be able to track"); + assert_eq!( + persister + .durable_asset_lock(&control_out_point) + .expect("the control row must be durable") + .status, + AssetLockStatus::Built, + ); + + // Remove the wallet, then re-import the SAME mnemonic/network. + manager + .remove_wallet(&wallet_id) + .await + .expect("remove should return the wallet"); + let replacement = manager + .create_wallet_from_seed_bytes( + network, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("re-import of the same mnemonic should succeed"); + + assert_eq!( + replacement.wallet_id(), + wallet_id, + "the re-import must reuse the deterministic id — otherwise this \ + test is not exercising the collision the fix is about" + ); + assert!( + !Arc::ptr_eq(replacement.asset_locks(), &retained), + "the re-import must have built a fresh asset-lock manager with \ + its own ordering mutex — that is what makes the retained handle \ + dangerous" + ); + + // The replacement wallet tracks a row of its own. This is the + // state a stale handle must not be able to reach. + let replacement_out_point = OutPoint { + txid: dashcore::Txid::from_raw_hash(dashcore::hashes::Hash::all_zeros()), + vout: 7, + }; + replacement + .asset_locks() + .track_asset_lock(tracked_lock( + replacement_out_point, + AssetLockStatus::Broadcast, + )) + .await + .expect("the replacement's own manager must be live"); + + let rounds_before_stale_attempts = persister.asset_lock_rounds(); + + // Every status mutate→enqueue primitive refuses through the + // retired handle. + let track = retained + .track_asset_lock(tracked_lock(control_out_point, AssetLockStatus::Built)) + .await; + assert!( + matches!(track, Err(PlatformWalletError::AssetLockManagerInactive(_))), + "a retired handle must not insert rows into the replacement \ + wallet, got {track:?}" + ); + + let untrack = retained.untrack_asset_lock(&replacement_out_point).await; + assert!( + matches!( + untrack, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "a retired handle must not untrack the replacement wallet's row — \ + the changeset's `removed` set would DELETE it, got {untrack:?}" + ); + + let advance = retained + .advance_asset_lock_status(&replacement_out_point, AssetLockStatus::ChainLocked, None) + .await; + assert!( + matches!( + advance, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "a retired handle must not advance the replacement wallet's row, \ + got {advance:?}" + ); + + let promote = retained + .promote_built_to_broadcast(&replacement_out_point) + .await; + assert!( + matches!( + promote, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "a retired handle must not promote the replacement wallet's row, \ + got {promote:?}" + ); + + let consume = retained.consume_asset_lock(&replacement_out_point).await; + assert!( + matches!( + consume, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "a retired handle must not consume the replacement wallet's row, \ + got {consume:?}" + ); + + // Neither in-memory nor durable replacement state moved. + { + let wm = manager.wallet_manager.read().await; + let info = wm + .get_wallet_info(&wallet_id) + .expect("the replacement wallet is registered"); + let row = info + .tracked_asset_locks + .get(&replacement_out_point) + .expect("the replacement's row must still be tracked"); + assert_eq!( + row.status, + AssetLockStatus::Broadcast, + "no refused operation may have mutated the replacement row" + ); + assert!( + !info.tracked_asset_locks.contains_key(&control_out_point), + "the retired handle's re-track must not have injected a row \ + into the replacement wallet" + ); + } + assert_eq!( + persister.asset_lock_rounds(), + rounds_before_stale_attempts, + "no refused operation may enqueue through the SHARED persister — \ + changesets are last-write-wins and `removed` deletes rows, so a \ + single stale round is enough to corrupt the replacement wallet" + ); + assert_eq!( + persister + .durable_asset_lock(&replacement_out_point) + .expect("the replacement's durable row must survive") + .status, + AssetLockStatus::Broadcast, + ); + + // Idempotency of the removal path itself is unchanged: a second + // removal of a wallet that is gone still reports WalletNotFound + // (the FFI maps that to ok), and retiring an already-retired + // manager is a no-op rather than a panic or a hang. + let unknown = [0xABu8; 32]; + assert!( + matches!( + manager.remove_wallet(&unknown).await, + Err(PlatformWalletError::WalletNotFound(_)) + ), + "removing an unknown wallet must still surface WalletNotFound" + ); + retained.deactivate().await; + } +} diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 5b3b8970297..6fad892707c 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -431,6 +431,14 @@ impl AssetLockManager { // dropped before the broadcast (only snapshot ordering needs // serializing; the UI's own single-flight guard is NOT sufficient — // a dismissed sheet's unstructured task keeps running). + // Fail a stale handle before spending a build (which allocates a + // funding index and reserves inputs) on a wallet that is gone. + // Advisory only — the authoritative refusal is the same check + // under `status_persist_serial` inside `track_asset_lock` and + // `promote_built_to_broadcast`, since a removal can land during + // the build or the broadcast await below. + self.ensure_active()?; + // Test-only occupancy gauge for the serialization gate (see // `build_serial_gate`). RAII so every exit path — including the // pre-broadcast aborts below — decrements. @@ -501,7 +509,11 @@ impl AssetLockManager { // 2. Track as Built and queue the changeset onto the persister // so a crash after broadcast leaves a row we can recover from. // `track_asset_lock` queues the changeset itself, as one - // serialized unit with the in-memory insert. + // serialized unit with the in-memory insert. It also re-checks + // the manager's lifecycle state under the ordering mutex, so a + // wallet removed during the build above aborts here — before a + // transaction reaches the wire and before a row is written that + // a replacement wallet would inherit. let _cs_built = self .track_asset_lock(TrackedAssetLock { out_point, @@ -513,7 +525,7 @@ impl AssetLockManager { status: AssetLockStatus::Built, proof: None, }) - .await; + .await?; tracing::debug!( %txid, @@ -533,7 +545,26 @@ impl AssetLockManager { if matches!(e, crate::broadcaster::BroadcastError::Rejected { .. }) { // `untrack_asset_lock` queues the changeset itself, as one // serialized unit with the in-memory removal. - let cs_untrack = self.untrack_asset_lock(&out_point).await; + // + // It refuses outright once the wallet has been removed from + // the manager. Treat that exactly like the untrack guard + // below: skip the release. The wallet whose reservation this + // call took no longer exists, and `wallet_id` now resolves to + // whatever replacement was registered under the same + // deterministic id — releasing there would free inputs the + // replacement never reserved. + let removed_row = match self.untrack_asset_lock(&out_point).await { + Ok(cs_untrack) => cs_untrack.removed.contains(&out_point), + Err(untrack_err) => { + tracing::warn!( + %txid, + error = %untrack_err, + "rejected broadcast could not untrack the Built row; \ + leaving the funding reservation alone" + ); + false + } + }; // Release only when the Built row was actually removed. If // the untrack guard fired instead — a concurrent // `resume_asset_lock` advanced the row past `Built`, positive @@ -541,7 +572,7 @@ impl AssetLockManager { // the inputs must stay reserved exactly like a `MaybeSent` // outcome, or the still-tracked row would be resumable while // its inputs are re-spendable. - if cs_untrack.removed.contains(&out_point) { + if removed_row { crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, &self.wallet_id, @@ -2467,4 +2498,204 @@ mod tests { durable.status ); } + + /// Regression (lifecycle): retiring an `AssetLockManager` must be a + /// BARRIER around the mutate→enqueue unit, not a flag flipped + /// whenever the removal happens to run. + /// + /// Two properties, both asserted here on a rendezvous rather than a + /// sleep: + /// + /// 1. **Deactivation waits.** A promoter parked between its + /// compare-and-set and its enqueue holds `status_persist_serial`. + /// `deactivate` must queue behind it — if it could flip the flag + /// in that window, the promoter would resume, fail its own + /// (already-taken) check or, worse, be interrupted with the row + /// mutated in memory and its changeset never handed to the + /// persister: exactly the mid-unit tear this mutex exists to + /// prevent. + /// 2. **Nothing lands afterwards.** Once `deactivate` returns, every + /// status mutate→enqueue primitive refuses with + /// `AssetLockManagerInactive` and queues nothing — including the + /// ones whose callers only arrive here after an unbounded + /// `broadcast` / proof wait, which is the span a `remove_wallet` + /// plus a same-mnemonic re-import completes inside. + /// + /// The rendezvous is the production `status_serial_waiters` gauge: + /// while this test's promoter provably holds the mutex, a waiter + /// count of 1 means `deactivate` came to rest at the boundary and + /// cannot have flipped the flag. A sleep would have proved nothing — + /// "the flag is still set" could equally mean the task had not been + /// scheduled yet. + #[tokio::test] + async fn deactivation_waits_for_the_in_flight_unit_then_refuses_every_later_mutation() { + let broadcaster = Arc::new(CountingMaybeSentBroadcaster { + call_count: AtomicUsize::new(0), + }); + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + // `MaybeSent` leaves the row at `Built` — the state a promotion + // acts on. + let _ = manager + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + let out_point = { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let (op, lock) = info + .tracked_asset_locks + .iter() + .next() + .expect("built row tracked"); + assert_eq!(lock.status, AssetLockStatus::Built); + *op + }; + + // 1. Park a promoter post-CAS / pre-enqueue. It holds + // `status_persist_serial` for the whole parked window. + let arrived = Arc::new(Notify::new()); + let release = Arc::new(Notify::new()); + *manager + .promote_post_cas_gate + .lock() + .expect("promote post-CAS gate mutex") = Some(PromotePostCasGate { + arrived: Arc::clone(&arrived), + release: Arc::clone(&release), + }); + let manager_promoter = Arc::clone(&manager); + let promoter = tokio::spawn(async move { + manager_promoter + .promote_built_to_broadcast(&out_point) + .await + }); + arrived.notified().await; + + let queued_before_deactivate = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + + // 2. Start the removal-side retirement. It must block. + let manager_deactivate = Arc::clone(&manager); + let deactivator = tokio::spawn(async move { manager_deactivate.deactivate().await }); + + // Rendezvous on `deactivate` REACHING the ordering boundary. + manager.await_status_serial_waiters(1).await; + + assert!( + manager.active.load(Ordering::SeqCst), + "`deactivate` must not retire the manager while an in-flight \ + mutate→enqueue unit still holds `status_persist_serial` — it \ + would strand the promoter with the row mutated in memory and \ + its changeset never enqueued" + ); + assert!( + manager.status_persist_serial.try_lock().is_err(), + "the parked promoter must still hold the ordering mutex — \ + otherwise the assertion above proves nothing about the barrier" + ); + + // 3. Release the promoter; its unit must complete in full. + release.notify_one(); + let promotion = promoter + .await + .expect("promoter task joined") + .expect("a promotion that started before the removal must complete"); + assert!( + matches!(promotion, super::BuiltPromotion::Promoted(_)), + "the in-flight promotion must have promoted the row, got {promotion:?}" + ); + deactivator.await.expect("deactivator task joined"); + + assert!( + persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len() + > queued_before_deactivate, + "the in-flight unit's changeset must have reached the persister \ + before deactivation completed — a retirement that cut in would \ + leave memory ahead of the durable row" + ); + assert_eq!( + persistence + .durable_asset_lock(&out_point) + .expect("the promoted row must be durable") + .status, + AssetLockStatus::Broadcast, + ); + + // 4. Every status mutate→enqueue primitive now refuses, and + // queues nothing. + let queued_after_deactivate = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + + let advance = manager + .advance_asset_lock_status(&out_point, AssetLockStatus::ChainLocked, None) + .await; + assert!( + matches!( + advance, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "a finalize arriving after retirement must be refused, got {advance:?}" + ); + + let promote = manager.promote_built_to_broadcast(&out_point).await; + assert!( + matches!( + promote, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "a promotion arriving after retirement must be refused, got {promote:?}" + ); + + let untrack = manager.untrack_asset_lock(&out_point).await; + assert!( + matches!( + untrack, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "an untrack arriving after retirement must be refused — it would \ + DELETE the durable row, got {untrack:?}" + ); + + let consume = manager.consume_asset_lock(&out_point).await; + assert!( + matches!( + consume, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "a consume arriving after retirement must be refused, got {consume:?}" + ); + + assert_eq!( + persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(), + queued_after_deactivate, + "no refused operation may enqueue anything — a retired manager \ + that still reaches the shared persister can overwrite or delete \ + a replacement wallet's rows" + ); + assert!( + persistence.removed_outpoints().is_empty(), + "the refused untrack must not have queued a row deletion" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index 4582a2464dc..e801f24f904 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -4,12 +4,14 @@ //! waiting for proofs, and tracking lifecycle status. Shared across sub-wallets //! via `Arc`. +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use tokio::sync::{Notify, RwLock}; use crate::broadcaster::TransactionBroadcaster; use crate::changeset::changeset::AssetLockChangeSet; +use crate::error::PlatformWalletError; use crate::wallet::persister::WalletPersister; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; @@ -187,6 +189,52 @@ pub struct AssetLockManager { /// makes the hook a no-op. #[cfg(test)] pub(super) resume_pre_promote_gate: std::sync::Mutex>, + /// Whether this manager may still mutate and persist asset-lock + /// state. Set once, `true` → `false`, by + /// [`deactivate`](Self::deactivate) when the owning wallet is + /// removed from its `PlatformWalletManager`. + /// + /// # Why a manager can outlive its wallet + /// + /// `AssetLockManager` resolves its target by `wallet_id` alone, + /// through the *shared* `WalletManager`. Wallet ids are + /// deterministic in (seed, network), so removing a wallet and + /// re-importing the same mnemonic re-creates the identical id over + /// a brand-new `PlatformWalletInfo` — and `PlatformWallet::new` + /// builds a brand-new `AssetLockManager` with its own + /// [`status_persist_serial`](Self::status_persist_serial). Any + /// `Arc` retained across that removal (an FFI + /// `asset_lock_manager` handle the host never destroyed, or a + /// resume task still parked in `wait_for_proof`) therefore becomes + /// live again against the *replacement* wallet's rows. Two managers + /// would then mutate and enqueue the same row under two different + /// mutexes, which is exactly the unserialized mutate→enqueue + /// interleave `status_persist_serial` exists to prevent — only now + /// across instances, where a single mutex cannot see it. + /// + /// # Why the check must live under `status_persist_serial` + /// + /// A bare `if !active { return }` before an `await` is racy: the + /// removal can land in the window between the check and the + /// mutation. So the authoritative test is taken AFTER acquiring + /// `status_persist_serial` and before the wallet lookup (see + /// [`ensure_active_under_serial`](Self::ensure_active_under_serial)), + /// and `deactivate` flips the flag while HOLDING that same mutex. + /// That gives both halves of the guarantee: + /// + /// - removal waits for any in-flight mutate→enqueue unit to finish + /// (it cannot take the mutex until the holder releases it), so no + /// changeset is stranded mid-unit; + /// - every operation that starts, or resumes from an await, after + /// the flip observes `false` before touching the wallet row or + /// the persister. + /// + /// `SeqCst` rather than `Relaxed` because the flag is also read + /// outside the mutex by the cheap early-out checks on the public + /// entry points; those are advisory (they only avoid wasted work), + /// but keeping one ordering for a single-writer flag costs nothing + /// here and avoids reasoning about two. + pub(super) active: AtomicBool, /// Test-only pause point inside /// [`promote_and_queue_built_to_broadcast`](Self::promote_and_queue_built_to_broadcast), /// between the compare-and-set mutation and the persistence enqueue. @@ -217,6 +265,7 @@ impl AssetLockManager { persister, build_persist_serial: tokio::sync::Mutex::new(()), status_persist_serial: tokio::sync::Mutex::new(()), + active: AtomicBool::new(true), #[cfg(test)] status_serial_waiters: std::sync::atomic::AtomicUsize::new(0), #[cfg(test)] @@ -267,6 +316,88 @@ impl AssetLockManager { guard } + /// Retire this manager: no later operation may mutate or persist + /// asset-lock state through it. + /// + /// Called by + /// [`PlatformWalletManager::remove_wallet`](crate::manager::PlatformWalletManager::remove_wallet) + /// BEFORE the wallet's entry is dropped from the shared + /// `WalletManager`, so a retained handle can never observe (let + /// alone write) the `PlatformWalletInfo` that a later re-import of + /// the same mnemonic installs under the same deterministic + /// `wallet_id`. See [`active`](Self::active) for why a manager can + /// outlive its wallet at all. + /// + /// The flag is flipped while HOLDING + /// [`status_persist_serial`](Self::status_persist_serial), which is + /// what makes the retirement a barrier rather than a hint: + /// + /// - it cannot take the mutex until any in-flight mutate→enqueue + /// unit has released it, so removal never interrupts one halfway + /// (mutated in memory, changeset not yet handed to the persister); + /// - once it returns, every subsequent acquirer — including + /// operations that had already started and were parked on + /// `broadcast` / `wait_for_proof` — sees `false` at + /// [`ensure_active_under_serial`](Self::ensure_active_under_serial) + /// before it touches the wallet row or the persister. + /// + /// Idempotent: a second call is a no-op (it still takes the mutex, + /// so it still waits out any in-flight unit). + /// + /// Must NOT be called while holding the `wallet_manager` lock — the + /// mutate→enqueue units this waits on acquire `wallet_manager` + /// themselves, so doing so would invert the documented + /// `status_persist_serial → wallet_manager` order and deadlock. + pub(crate) async fn deactivate(&self) { + let _serial = self.lock_status_persist_serial().await; + let was_active = self.active.swap(false, Ordering::SeqCst); + if was_active { + tracing::debug!( + wallet_id = %hex::encode(self.wallet_id), + "AssetLockManager deactivated: its wallet was removed from the manager" + ); + } + } + + /// The authoritative stale-handle check, taken by every asset-lock + /// status mutate→enqueue primitive AFTER it has acquired + /// [`status_persist_serial`](Self::status_persist_serial) and + /// BEFORE it looks the wallet up. + /// + /// Ordering is the whole point: the caller already holds the mutex + /// that [`deactivate`](Self::deactivate) must take to flip the + /// flag, so a `true` observed here cannot go stale for the + /// remainder of the critical section — the mutation and its + /// enqueue both complete against the wallet this manager was built + /// for. Checking before the mutex (as the public entry points also + /// do, cheaply) is only an early-out; it can be invalidated by a + /// removal landing during the very next `await`. + pub(super) fn ensure_active_under_serial( + &self, + _serial: &tokio::sync::MutexGuard<'_, ()>, + ) -> Result<(), PlatformWalletError> { + self.ensure_active() + } + + /// Cheap advisory activity check for public entry points + /// (`resume_asset_lock`, `broadcast_funded_asset_lock`, …), so a + /// stale handle fails immediately instead of doing a build or an + /// unbounded proof wait whose mutation will be refused anyway. + /// + /// NOT sufficient on its own — a removal can land between this + /// check and any later await. The under-mutex + /// [`ensure_active_under_serial`](Self::ensure_active_under_serial) + /// is what actually closes the race. + pub(super) fn ensure_active(&self) -> Result<(), PlatformWalletError> { + if self.active.load(Ordering::SeqCst) { + Ok(()) + } else { + Err(PlatformWalletError::AssetLockManagerInactive(hex::encode( + self.wallet_id, + ))) + } + } + /// Test-only: wait until at least `n` tasks are blocked on /// [`status_persist_serial`](Self::status_persist_serial). /// diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index c2350d1dda1..2a14d4a5463 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -34,6 +34,22 @@ impl AssetLockManager { /// on-chain context from `ManagedWalletInfo` to determine the correct /// status (and constructs a `ChainAssetLockProof` if the TX is in a /// chain-locked block). + /// + /// # Lifecycle + /// + /// Silently no-ops (with a warning) when the owning wallet has been + /// removed from the `PlatformWalletManager`. This path returns `()` + /// — it is a best-effort catch-up whose every other failure mode is + /// already logged-and-dropped — so a stale handle is refused the + /// same way rather than by a signature change. + /// + /// Like the async mutators, the authoritative check happens AFTER + /// `status_persist_serial` is taken and before the commit-phase + /// wallet lookup: phase 2 resolves status without any lock held and + /// may call into host persistence, which is more than enough time + /// for a removal (and a re-import re-creating the same + /// deterministic id) to land. The advisory pre-check up front only + /// saves that work. #[allow(clippy::too_many_arguments)] pub fn recover_asset_lock_blocking( &self, @@ -45,6 +61,16 @@ impl AssetLockManager { out_point: OutPoint, proof: Option, ) { + if let Err(e) = self.ensure_active() { + tracing::warn!( + outpoint = %out_point, + error = %e, + "recover_asset_lock_blocking: refusing to recover through a \ + retired asset-lock manager" + ); + return; + } + // Phase 1 (lock held): claim the tracked-asset-lock slot and // pull the in-memory record out so the lookup work is // bounded to a single hashmap fetch. @@ -96,6 +122,21 @@ impl AssetLockManager { // as callable only OUTSIDE a tokio async context. let _serial = self.status_persist_serial.blocking_lock(); + // Authoritative stale-handle check, under the same mutex + // `deactivate` must hold to retire this manager — so a removal + // racing phase 2 either finished (and this insert is refused) + // or is still waiting for this critical section to end. + if let Err(e) = self.ensure_active_under_serial(&_serial) { + tracing::warn!( + outpoint = %out_point, + error = %e, + "recover_asset_lock_blocking: wallet was removed while resolving \ + the lock's status — dropping the recovery instead of writing to \ + replacement wallet state" + ); + return; + } + // We re-check `tracked_asset_locks.contains_key` because // another caller could have raced in during phase 2 — first // writer wins. @@ -228,6 +269,14 @@ impl AssetLockManager { ) -> Result<(dpp::prelude::AssetLockProof, DerivationPath), PlatformWalletError> { tracing::info!(outpoint = %out_point, ?timeout, "resume_asset_lock: entered"); + // Fail a stale handle before doing any work. Advisory only — + // this call goes on to await a broadcast and a proof, so the + // removal it is meant to catch can equally land afterwards. The + // guarantee comes from the same check inside + // `promote_built_to_broadcast` / `advance_asset_lock_status`, + // taken under `status_persist_serial`. + self.ensure_active()?; + // 1. Look up the tracked lock — snapshot the fields we need. let (tx, mut status, mut existing_proof, account_index) = { let wm = self.wallet_manager.read().await; diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index 200e7b6ee03..64a158d6df6 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -62,8 +62,21 @@ impl AssetLockManager { /// lock drops a resume can promote it to `Broadcast` and enqueue /// that — and an unserialized `Built` enqueue landing afterwards /// would regress the durable row to the pre-broadcast state. - pub(crate) async fn track_asset_lock(&self, lock: TrackedAssetLock) -> AssetLockChangeSet { + /// + /// # Lifecycle + /// + /// Errors with + /// [`AssetLockManagerInactive`](PlatformWalletError::AssetLockManagerInactive) + /// when the owning wallet has been removed. Checked under the + /// ordering mutex, so a removal racing this call either completes + /// first (and this insert is refused) or waits for it (and the row + /// belongs to the wallet being removed, not to a replacement). + pub(crate) async fn track_asset_lock( + &self, + lock: TrackedAssetLock, + ) -> Result { let _serial = self.lock_status_persist_serial().await; + self.ensure_active_under_serial(&_serial)?; let cs = { let mut wm = self.wallet_manager.write().await; @@ -77,7 +90,7 @@ impl AssetLockManager { }; self.queue_asset_lock_changeset(cs.clone()); - cs + Ok(cs) } /// Remove a tracked asset lock whose funding transaction was @@ -112,8 +125,22 @@ impl AssetLockManager { /// row-deleting `removed` enqueue to land after a concurrent /// finalize's proof-bearing snapshot, Swift would delete the row /// that snapshot had just written. - pub(crate) async fn untrack_asset_lock(&self, out_point: &OutPoint) -> AssetLockChangeSet { + /// + /// # Lifecycle + /// + /// Errors with + /// [`AssetLockManagerInactive`](PlatformWalletError::AssetLockManagerInactive) + /// when the owning wallet has been removed — checked under the + /// ordering mutex. This is the arm that matters most for a stale + /// handle: the `removed` set DELETES the durable row, so a retired + /// manager reaching this method after a re-import would erase a + /// replacement wallet's freshly-tracked lock. + pub(crate) async fn untrack_asset_lock( + &self, + out_point: &OutPoint, + ) -> Result { let _serial = self.lock_status_persist_serial().await; + self.ensure_active_under_serial(&_serial)?; let cs = { let mut wm = self.wallet_manager.write().await; @@ -136,7 +163,7 @@ impl AssetLockManager { }; self.queue_asset_lock_changeset(cs.clone()); - cs + Ok(cs) } /// Mark a tracked asset lock as @@ -174,6 +201,11 @@ impl AssetLockManager { /// nothing queued. /// - `Err(WalletNotFound)` — the wallet id is unknown to the /// manager. Always a programmer error / stale handle. + /// - `Err(AssetLockManagerInactive)` — the owning wallet was + /// removed from the `PlatformWalletManager`. Checked under the + /// ordering mutex, before the wallet lookup, so a re-imported + /// wallet that happens to share the same deterministic id cannot + /// have its rows consumed through the retired handle. /// /// **Why queue internally** (unlike `track_asset_lock` / /// `advance_asset_lock_status`, which return a changeset and let @@ -194,6 +226,10 @@ impl AssetLockManager { // concurrent status writer's older one (see // `status_persist_serial`). Acquired BEFORE `wallet_manager`. let _serial = self.lock_status_persist_serial().await; + // Authoritative stale-handle check: under the mutex, before the + // wallet lookup below can resolve a REPLACEMENT wallet that + // re-import installed under the same deterministic id. + self.ensure_active_under_serial(&_serial)?; // Build the changeset under the write lock, then release the // lock before queueing — `queue_asset_lock_changeset` calls @@ -265,6 +301,16 @@ impl AssetLockManager { /// Both `Built` → `Broadcast` callers (create and resume) go through /// here, so neither can reorder against the other or against /// `advance_asset_lock_status`. + /// + /// # Lifecycle + /// + /// Errors with + /// [`AssetLockManagerInactive`](PlatformWalletError::AssetLockManagerInactive) + /// when the owning wallet has been removed. Both callers reach this + /// method only after an unbounded `broadcast(&tx)` await, so the + /// under-mutex check — not the entry-point one — is what stops a + /// resume that began before the removal from writing to a + /// replacement wallet's row. pub(crate) async fn promote_built_to_broadcast( &self, out_point: &OutPoint, @@ -272,6 +318,7 @@ impl AssetLockManager { // Hold the ordering mutex across mutate → enqueue. Acquired // BEFORE `wallet_manager` (see the field's lock-ordering note). let _serial = self.lock_status_persist_serial().await; + self.ensure_active_under_serial(&_serial)?; let promotion = self.compare_and_set_built_to_broadcast(out_point).await?; @@ -353,6 +400,15 @@ impl AssetLockManager { /// a proof-bearing `InstantSendLocked` / `ChainLocked` snapshot can /// be enqueued BEFORE a concurrently-delayed promoter enqueues its /// older `Broadcast + None` one, regressing the durable row. + /// + /// # Lifecycle + /// + /// Errors with + /// [`AssetLockManagerInactive`](PlatformWalletError::AssetLockManagerInactive) + /// when the owning wallet has been removed. Every caller reaches + /// this after an unbounded proof wait, which is precisely the span + /// a `remove_wallet` + re-import can complete inside — so the + /// under-mutex check is the one that matters here. pub(crate) async fn advance_asset_lock_status( &self, out_point: &OutPoint, @@ -362,6 +418,7 @@ impl AssetLockManager { // Hold the ordering mutex across mutate → enqueue. Acquired // BEFORE `wallet_manager` (see the field's lock-ordering note). let _serial = self.lock_status_persist_serial().await; + self.ensure_active_under_serial(&_serial)?; // Scoped so the wallet write guard is released before the // synchronous persister call below. From d59ddc4f530c8084b6a806c96e9e38ba96a6e1bb Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 25 Jul 2026 08:07:15 -0500 Subject: [PATCH 07/12] fix(platform-wallet): refuse a delayed asset-lock status/proof downgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `advance_asset_lock_status` assigned unconditionally, and it is the single write point for asset-lock status. Serialization does not close this: `status_persist_serial` makes each writer's mutation and enqueue one indivisible unit, so the durable order matches the in-memory order — but the two proof-bearing writes come from INDEPENDENT waiters, so nothing orders them against each other. `wait_for_proof` returns whichever SPV event fires first, while the IS->CL upgrade paths (`upgrade_to_chain_lock_proof` after a Platform IS rejection, `validate_or_upgrade_proof` on a rotated quorum) can complete a `ChainLocked` write with an earlier IS waiter still parked. Released afterwards, that IS waiter's write was internally consistent and perfectly serialized — and still regressed the row. Both halves of the regression hurt. The status fell below the `>= InstantSendLocked` predicates the catch-up scanner and the ready-to-fund UI filter use. The proof swap is worse: the caller passes `Some(is_proof)`, so the ChainLock proof — the one that survives quorum rotation, and the one an IS-rejection retry had upgraded TO — was replaced by the IS proof that rejection was about. A restart then reloaded the weaker proof from the durable row and re-armed the very rejection the upgrade resolved. Guard the write under the same `status_persist_serial` the mutation and enqueue already hold, so the status it compares against is the one it is about to overwrite. A strictly-lower-rank `new_status` mutates nothing, replaces no proof, and enqueues nothing. Ordering comes from an explicit `AssetLockStatus::lifecycle_rank`, an exhaustive match on named variants rather than `as u8` on the declaration order. The two agree today, but that order is separately load-bearing for the FFI discriminants (`status_from_u8`) and the SQLite label domain, and this is a *semantic* claim about the lifecycle — tying it to declaration position would let a reordering silently redefine which writes count as downgrades. No `_` arm, so a new variant is a compile error here. Caller-return semantics are deliberately unchanged. A refusal is `Ok(empty changeset)`, not an error: every production caller returns/uses the proof it already obtained and ignores the changeset, so the delayed IS caller may still submit with its valid IS proof. Only SHARED state — the in-memory row and the durable row — is held at the stronger proof. Equal-rank writes are NOT refused, which keeps two live shapes working: attaching the first proof to a row `resolve_status_with_in_memory` marked `InstantSendLocked` with `None` (it has no IS-lock data), and re-writing `ChainLocked` with a freshly-upgraded proof at a newer height. Two regression tests, no sleep in either: - `a_late_instant_send_write_cannot_downgrade_a_chain_locked_row` parks an IS writer on a new test-only gate placed BEFORE the ordering mutex (a gate after it would invert the interleave — the parked writer would hold the lock and its IS write would land first, a legal forward advance), finalizes the same row to `ChainLocked` + chain proof and awaits that call so the stronger write is provably complete, then releases the IS writer. Asserts in-memory AND durable state both stay `ChainLocked` with the chain proof, that nothing was enqueued, and that the refusal is `Ok` with an empty changeset. The gate is one-shot so the competing finalize — same method — runs straight through instead of deadlocking against the gate it races. - `monotonicity_guard_allows_forward_and_same_status_proof_writes` pins the other direction: forward transitions, equal-rank proof attachment, and equal-rank proof refresh all still mutate and enqueue. Negative controls: disabling the guard fails the first test three times over, independently — on the empty-changeset assertion, on the in-memory status, and on the durable status. Writing the guard as `<=` instead of `<` fails the second test on the equal-rank proof attachment. Scope is monotonic asset-lock status/proof writes only. The separate manager-lifecycle fix is unchanged. Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 378 ++++++++++++++++++ .../src/wallet/asset_lock/manager.rs | 39 ++ .../src/wallet/asset_lock/sync/tracking.rs | 104 ++++- .../src/wallet/asset_lock/tracked.rs | 90 +++++ 4 files changed, 606 insertions(+), 5 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 6fad892707c..40a04350196 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -2698,4 +2698,382 @@ mod tests { "the refused untrack must not have queued a row deletion" ); } + + /// Regression (monotonicity): a delayed `InstantSendLocked + IS proof` + /// write must not downgrade a row a concurrent flow already finalized + /// to `ChainLocked + CL proof` — in memory or durably. + /// + /// Serialization alone does not close this. `status_persist_serial` + /// makes each writer's mutation and enqueue one indivisible unit, so + /// the durable order matches the in-memory order — but the two + /// proof-bearing writes come from INDEPENDENT waiters + /// (`wait_for_proof` returns whichever SPV event fires first; the + /// IS→CL upgrade paths can finalize a ChainLock while an earlier IS + /// waiter is still parked), so nothing orders them. Released after + /// the finalize, the IS waiter's write was internally consistent and + /// perfectly serialized — and still strictly regressed the row: the + /// status fell below the `>= InstantSendLocked` predicates the + /// catch-up scanner and the "ready to fund" UI filter on, and, + /// because the caller passes `Some(is_proof)`, the ChainLock proof + /// was swapped out for the IS one that a rejection retry had + /// upgraded AWAY from. A restart then reloaded the weaker proof. + /// + /// Interleave, with no sleep anywhere in it: + /// + /// 1. an IS writer enters `advance_asset_lock_status` and parks on + /// the pre-lock gate — before the ordering mutex, so the finalize + /// below can run its whole unit rather than queueing behind it; + /// 2. the test finalizes the SAME row to `ChainLocked` + chain proof + /// and awaits that call, so the stronger write is provably + /// complete and enqueued before the IS writer resumes; + /// 3. the IS writer is released and joined. + /// + /// Then both halves of the shared state must still read + /// `ChainLocked` + chain proof, and the refused write must have + /// enqueued nothing at all. + /// + /// Caller-return semantics are asserted too: the refusal is `Ok`, not + /// an error, and carries an empty changeset. The delayed caller's own + /// IS proof is still valid evidence for the submission it is about to + /// make (every production caller returns/uses the proof it already + /// holds, never anything from the changeset) — the guard constrains + /// SHARED state, not what the caller may do with the proof in hand. + #[tokio::test] + async fn a_late_instant_send_write_cannot_downgrade_a_chain_locked_row() { + use dpp::identity::state_transition::asset_lock_proof::chain::ChainAssetLockProof; + use dpp::identity::state_transition::asset_lock_proof::InstantAssetLockProof; + + use crate::wallet::asset_lock::manager::AdvancePreLockGate; + + // `MaybeSent` leaves the row tracked at `Built`; the test drives + // the status transitions itself. + let broadcaster = Arc::new(CountingMaybeSentBroadcaster { + call_count: AtomicUsize::new(0), + }); + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + let _ = manager + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + let out_point = { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let (op, lock) = info + .tracked_asset_locks + .iter() + .next() + .expect("built row tracked"); + assert_eq!(lock.status, AssetLockStatus::Built); + *op + }; + + // Advance to `Broadcast` first, so the IS write under test is a + // legal forward transition in isolation — its refusal below is + // then attributable to the concurrent finalize, not to the write + // being backwards on its own. + manager + .advance_asset_lock_status(&out_point, AssetLockStatus::Broadcast, None) + .await + .expect("Broadcast is a forward transition from Built"); + + let chain_proof = dpp::prelude::AssetLockProof::Chain(ChainAssetLockProof { + core_chain_locked_height: 4016, + out_point, + }); + let instant_proof = dpp::prelude::AssetLockProof::Instant(InstantAssetLockProof::new( + dashcore::ephemerealdata::instant_lock::InstantLock::default(), + dashcore::Transaction { + version: 3, + lock_time: 0, + input: vec![], + output: vec![], + special_transaction_payload: None, + }, + 0, + )); + assert_ne!( + instant_proof, chain_proof, + "the two proofs must be distinguishable for the assertions below \ + to mean anything" + ); + + // 1. Park an `InstantSendLocked` writer before the ordering + // mutex. The gate is one-shot (taken, not cloned), so the + // finalize in step 2 — same method — runs straight through + // instead of parking too. + let arrived = Arc::new(Notify::new()); + let release = Arc::new(Notify::new()); + *manager + .advance_pre_lock_gate + .lock() + .expect("advance pre-lock gate mutex") = Some(AdvancePreLockGate { + arrived: Arc::clone(&arrived), + release: Arc::clone(&release), + }); + + let manager_is_writer = Arc::clone(&manager); + let is_writer_proof = instant_proof.clone(); + let is_writer = tokio::spawn(async move { + manager_is_writer + .advance_asset_lock_status( + &out_point, + AssetLockStatus::InstantSendLocked, + Some(is_writer_proof), + ) + .await + }); + + // The IS writer has provably entered the method and cannot + // proceed. This is the arrival signal, not a sleep: it fires from + // inside the call under test, so "the finalize below races a + // parked IS write" is a fact rather than a scheduling hope. + arrived.notified().await; + + // 2. Finalize the row to `ChainLocked` + chain proof and AWAIT + // it. Awaiting inline is safe (and is the point): the IS + // writer parks before taking `status_persist_serial`, so it + // holds nothing this call needs. On return the stronger write + // is complete — mutated in memory and enqueued. + let finalize_cs = manager + .advance_asset_lock_status( + &out_point, + AssetLockStatus::ChainLocked, + Some(chain_proof.clone()), + ) + .await + .expect("the ChainLock finalize must succeed"); + assert!( + !::is_empty( + &finalize_cs + ), + "the finalize must have produced a real changeset — otherwise the \ + race below is not against a completed stronger write" + ); + + let queued_after_finalize = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + + // 3. Release the delayed IS writer. + release.notify_one(); + let refused = is_writer + .await + .expect("IS writer task joined") + .expect("a refused downgrade must be reported as Ok, not an error"); + assert!( + ::is_empty( + &refused + ), + "the refused downgrade must return an EMPTY changeset — a populated \ + one would be replayed as an `InstantSendLocked + IS proof` row by \ + any caller that queued it, reintroducing the regression" + ); + + // In-memory state kept the stronger status AND the stronger proof. + { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let lock = info + .tracked_asset_locks + .get(&out_point) + .expect("row still tracked"); + assert_eq!( + lock.status, + AssetLockStatus::ChainLocked, + "a late InstantSendLocked write must not roll the in-memory row \ + back below ChainLocked — it would fall under the \ + `>= InstantSendLocked` predicates the catch-up scanner and the \ + ready-to-fund filter use" + ); + assert_eq!( + lock.proof.as_ref(), + Some(&chain_proof), + "the ChainLock proof must survive: it is what an IS-rejection \ + retry upgraded TO, and reinstating the IS proof re-arms the \ + very rejection that upgrade resolved" + ); + } + + // Durable state — replayed through the real downstream semantics + // (round order, last-write-wins merge, unconditional upsert) — + // agrees, and the refused write reached the persister not at all. + let durable = persistence + .durable_asset_lock(&out_point) + .expect("the finalized row must be durable"); + assert_eq!( + durable.status, + AssetLockStatus::ChainLocked, + "the durable row must stay ChainLocked; the load path treats a \ + regressed status as still-pending and would resume a lock whose \ + proof it already had" + ); + assert_eq!( + durable.proof.as_ref(), + Some(&chain_proof), + "the durable proof must stay the ChainLock one — a restart reloads \ + exactly this row" + ); + assert_eq!( + persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(), + queued_after_finalize, + "the refused downgrade must enqueue nothing at all; even a \ + correctly-ordered stale round is last-write-wins downstream" + ); + } + + /// The monotonicity guard must refuse only BACKWARD writes. Forward + /// transitions and equal-rank proof attachment/refresh — the shapes + /// the production callers actually depend on — must still mutate and + /// enqueue. + /// + /// Equal-rank matters as much as forward here, and for two live + /// reasons: `resolve_status_with_in_memory` classifies a tx with an + /// InstantSend context as `InstantSendLocked` with NO proof (it has + /// no IS-lock data), so the first real proof arrives as a same-status + /// write; and the IS→CL upgrade paths re-write `ChainLocked` with a + /// freshly-built ChainLock proof at a newer height. A guard written + /// as `<=` instead of `<` would silently drop both. + #[tokio::test] + async fn monotonicity_guard_allows_forward_and_same_status_proof_writes() { + use dpp::identity::state_transition::asset_lock_proof::chain::ChainAssetLockProof; + + let broadcaster = Arc::new(CountingMaybeSentBroadcaster { + call_count: AtomicUsize::new(0), + }); + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + let _ = manager + .create_funded_asset_lock_proof( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + let out_point = { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + *info + .tracked_asset_locks + .keys() + .next() + .expect("built row tracked") + }; + + // Forward, one rank at a time, ending on a proof-bearing write. + for status in [ + AssetLockStatus::Broadcast, + AssetLockStatus::InstantSendLocked, + ] { + let cs = manager + .advance_asset_lock_status(&out_point, status.clone(), None) + .await + .expect("a forward transition must succeed"); + assert!( + !::is_empty(&cs), + "the forward transition to {status:?} must produce a changeset" + ); + } + + // Same-status proof ATTACHMENT: the row is already + // `InstantSendLocked` with no proof (exactly what + // `resolve_status_with_in_memory` leaves behind), and the first + // real proof arrives at equal rank. + let chain_proof_low = dpp::prelude::AssetLockProof::Chain(ChainAssetLockProof { + core_chain_locked_height: 4016, + out_point, + }); + let attach = manager + .advance_asset_lock_status( + &out_point, + AssetLockStatus::InstantSendLocked, + Some(chain_proof_low.clone()), + ) + .await + .expect("same-status proof attachment must succeed"); + assert!( + !::is_empty( + &attach + ), + "attaching the first proof at equal rank must produce a changeset — \ + `resolve_status_with_in_memory` sets InstantSendLocked with no \ + proof, so this is how that row ever gets one" + ); + + // Forward to `ChainLocked`, then a same-status REFRESH with a + // ChainLock proof at a newer height — the IS→CL upgrade shape. + manager + .advance_asset_lock_status( + &out_point, + AssetLockStatus::ChainLocked, + Some(chain_proof_low), + ) + .await + .expect("advancing to ChainLocked must succeed"); + let chain_proof_high = dpp::prelude::AssetLockProof::Chain(ChainAssetLockProof { + core_chain_locked_height: 4129, + out_point, + }); + let refresh = manager + .advance_asset_lock_status( + &out_point, + AssetLockStatus::ChainLocked, + Some(chain_proof_high.clone()), + ) + .await + .expect("same-status proof refresh must succeed"); + assert!( + !::is_empty( + &refresh + ), + "re-writing ChainLocked with a freshly-upgraded proof must produce a \ + changeset — this is the IS-rejection retry path" + ); + + // Both halves reflect the refreshed proof. + { + let wm = manager.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + let lock = info + .tracked_asset_locks + .get(&out_point) + .expect("row still tracked"); + assert_eq!(lock.status, AssetLockStatus::ChainLocked); + assert_eq!( + lock.proof.as_ref(), + Some(&chain_proof_high), + "the refreshed proof must be the one in memory" + ); + } + let durable = persistence + .durable_asset_lock(&out_point) + .expect("the row must be durable"); + assert_eq!(durable.status, AssetLockStatus::ChainLocked); + assert_eq!( + durable.proof.as_ref(), + Some(&chain_proof_high), + "the refreshed proof must be the durable one" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index e801f24f904..357ccce8108 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -48,6 +48,28 @@ pub(super) struct PromotePostCasGate { pub(super) release: Arc, } +/// Test-only rendezvous for +/// [`AssetLockManager::advance_pre_lock_gate`]. `arrived` fires once an +/// [`advance_asset_lock_status`](AssetLockManager::advance_asset_lock_status) +/// call has entered but BEFORE it acquires +/// [`status_persist_serial`](AssetLockManager::status_persist_serial); +/// the writer then blocks on `release`. +/// +/// Parked *before* the mutex on purpose. The hazard the monotonicity +/// guard closes is a writer that is late relative to another writer's +/// whole mutate→enqueue unit — an IS-lock waiter released after a +/// ChainLock finalize has already completed. A gate placed after the +/// mutex could not produce that: the parked writer would hold the lock, +/// the ChainLock finalize would queue behind it, and the IS write would +/// land FIRST (a legal `Broadcast` → `InstantSendLocked` advance), which +/// is the opposite interleave. +#[cfg(test)] +#[derive(Clone)] +pub(super) struct AdvancePreLockGate { + pub(super) arrived: Arc, + pub(super) release: Arc, +} + /// Manages the full asset lock lifecycle: build, broadcast, proof, and tracking. /// /// Shared across sub-wallets via `Arc` so that any sub-wallet @@ -244,6 +266,21 @@ pub struct AssetLockManager { /// default) makes the hook a no-op. #[cfg(test)] pub(super) promote_post_cas_gate: std::sync::Mutex>, + /// Test-only pause point at the very top of + /// [`advance_asset_lock_status`](Self::advance_asset_lock_status), + /// before [`status_persist_serial`](Self::status_persist_serial) is + /// taken. Lets a test hold an `InstantSendLocked` writer there while + /// another flow finalizes the same row to `ChainLocked` with a + /// ChainLock proof — the delayed-downgrade interleave the + /// monotonicity guard exists to refuse. `None` (the default) makes + /// the hook a no-op. + /// + /// Consumed on arrival (taken, not cloned), so only the FIRST + /// advance parks: the finalize the test performs while the IS writer + /// waits goes through the same method and must not deadlock against + /// the gate it is racing. + #[cfg(test)] + pub(super) advance_pre_lock_gate: std::sync::Mutex>, } impl AssetLockManager { @@ -274,6 +311,8 @@ impl AssetLockManager { resume_pre_promote_gate: std::sync::Mutex::new(None), #[cfg(test)] promote_post_cas_gate: std::sync::Mutex::new(None), + #[cfg(test)] + advance_pre_lock_gate: std::sync::Mutex::new(None), } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index 64a158d6df6..96d84c37602 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -386,11 +386,60 @@ impl AssetLockManager { /// Returns an [`AssetLockChangeSet`] carrying a full snapshot of the /// updated entry. The changeset has ALREADY been queued for /// persistence before return; the value is surfaced so callers and - /// tests can inspect the diff. - /// - /// Assigns unconditionally — callers that race another writer for the - /// same row must gate the write themselves (see - /// [`promote_built_to_broadcast`](Self::promote_built_to_broadcast)). + /// tests can inspect the diff. When the write is refused as a + /// downgrade (below) the changeset is EMPTY and nothing was queued. + /// + /// Forward-only within the [`AssetLockStatus`] lifecycle — callers + /// that race another writer for the same row need no gate of their + /// own for the *backward* case. A `Built` → `Broadcast` race still + /// needs [`promote_built_to_broadcast`](Self::promote_built_to_broadcast), + /// which is a different question (both writers agree on the target + /// status; the loser must learn the row already advanced so it can + /// re-dispatch). + /// + /// # Monotonicity + /// + /// A write whose `new_status` ranks strictly BELOW the row's current + /// status (see [`AssetLockStatus::lifecycle_rank`]) mutates nothing, + /// replaces no proof, and enqueues no snapshot. + /// + /// This is not the same race the ordering mutex closes. The mutex + /// makes each writer's mutation and enqueue one indivisible unit, so + /// the durable order matches the in-memory order — but two writers + /// that are each internally consistent can still arrive in the wrong + /// ORDER, because the IS-lock and ChainLock waiters are independent: + /// `wait_for_proof` returns whichever SPV event fires first, and the + /// IS→CL upgrade paths (`upgrade_to_chain_lock_proof` after a + /// Platform IS rejection, `validate_or_upgrade_proof` on a stale + /// quorum) can complete a ChainLocked write while an earlier IS + /// waiter is still parked. Released afterwards, that IS waiter would + /// write `InstantSendLocked` over `ChainLocked` and — since it + /// passes `Some(is_proof)` — swap the ChainLock proof out for the IS + /// one, in memory and durably. + /// + /// Both halves matter. The status regression alone puts the row + /// below the `>= InstantSendLocked` predicates the catch-up scanner + /// and the "ready to fund" UI filter on. The proof swap is worse: + /// the ChainLock proof is the one that survives quorum rotation, and + /// it is what an IS rejection retry upgraded TO — reinstating the IS + /// proof re-arms exactly the rejection that upgrade resolved, and a + /// restart reloads the weaker proof from the durable row. + /// + /// **Caller-return semantics are deliberately unchanged.** A refused + /// caller gets `Ok(empty changeset)`, not an error: its own proof is + /// still valid evidence for the submission it is about to make, and + /// every production caller returns/uses the proof it already holds + /// rather than anything from the changeset. So the delayed IS caller + /// may go on submitting with its IS proof; only the SHARED state + /// (in-memory row and durable row) is held at the stronger proof. + /// + /// Equal-rank writes are NOT refused, so same-status proof + /// attachment and refresh keep working: attaching the first proof to + /// a row already marked `InstantSendLocked` by + /// `resolve_status_with_in_memory` (which sets that status with + /// `None`, since it has no IS-lock data), and re-writing + /// `ChainLocked` with a freshly-upgraded ChainLock proof at a newer + /// height. /// /// # Ordering /// @@ -415,6 +464,24 @@ impl AssetLockManager { new_status: AssetLockStatus, proof: Option, ) -> Result { + // Test-only pause BEFORE the ordering mutex, so a test can hold + // this writer while a competing finalize runs its whole + // mutate→enqueue unit to completion. One-shot: taken rather than + // cloned, so the competing finalize (same method) runs straight + // through. See `advance_pre_lock_gate`. + #[cfg(test)] + { + let gate = self + .advance_pre_lock_gate + .lock() + .expect("advance pre-lock gate mutex") + .take(); + if let Some(gate) = gate { + gate.arrived.notify_one(); + gate.release.notified().await; + } + } + // Hold the ordering mutex across mutate → enqueue. Acquired // BEFORE `wallet_manager` (see the field's lock-ordering note). let _serial = self.lock_status_persist_serial().await; @@ -433,6 +500,33 @@ impl AssetLockManager { out_point )) })?; + + // Monotonicity guard — see the `# Monotonicity` section. + // Checked here, under the same `status_persist_serial` the + // mutation and enqueue hold, so the status it reads is the + // one this unit is about to overwrite: no concurrent writer + // can finalize the row between the comparison and the + // assignment. + // + // Bail out with an EMPTY changeset rather than an error. The + // row is already at least as advanced as what this caller + // would have written, which is a success from the wallet + // state's point of view, and the caller's own proof stays + // usable for its pending submission. + if new_status.lifecycle_rank() < entry.status.lifecycle_rank() { + tracing::info!( + outpoint = %out_point, + current_status = ?entry.status, + refused_status = ?new_status, + current_has_proof = entry.proof.is_some(), + refused_carries_proof = proof.is_some(), + "advance_asset_lock_status: refusing a status downgrade — \ + keeping the row's stronger status and proof; the caller \ + may still use the proof it obtained" + ); + return Ok(AssetLockChangeSet::default()); + } + entry.status = new_status; if proof.is_some() { entry.proof = proof; diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs index 7998145080c..1448dcaba29 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs @@ -55,6 +55,47 @@ pub enum AssetLockStatus { Consumed, } +impl AssetLockStatus { + /// Position of this variant in the asset lock's forward-only + /// lifecycle, used to reject a delayed writer's *backward* status + /// write (see + /// [`advance_asset_lock_status`](crate::wallet::asset_lock::manager::AssetLockManager::advance_asset_lock_status)). + /// + /// A lock only ever moves forward: it is built, broadcast, covered + /// by an InstantSend lock, then by a ChainLock, then consumed. But + /// the two proof-bearing stages are produced by *independent* + /// waiters — an IS-lock arrives from one SPV event and a ChainLock + /// from another — so nothing about the call order guarantees the + /// stronger one is written last. A delayed + /// `InstantSendLocked + IS proof` write landing after a + /// `ChainLocked + CL proof` one would replace strictly better + /// evidence with weaker evidence, in memory and durably. Comparing + /// ranks is what lets the write be refused. + /// + /// Ranks are assigned by an exhaustive match on named variants + /// rather than by `as u8` on the declaration order. The two agree + /// today, but the enum's order is also load-bearing for the FFI + /// discriminants (`status_from_u8`) and the SQLite label domain, + /// and this ordering is a *semantic* claim about the lifecycle — + /// tying it to declaration position would let a future reordering + /// silently redefine which writes count as downgrades. The match + /// has no `_` arm, so adding a variant is a compile error here, the + /// same signal the enum's `#[non_exhaustive]` note describes. + /// + /// `Consumed` ranks highest: it is terminal, and its entry is + /// dropped from `tracked_asset_locks` outright (so in practice an + /// advance against it fails the tracked-row lookup first). + pub(crate) fn lifecycle_rank(&self) -> u8 { + match self { + AssetLockStatus::Built => 0, + AssetLockStatus::Broadcast => 1, + AssetLockStatus::InstantSendLocked => 2, + AssetLockStatus::ChainLocked => 3, + AssetLockStatus::Consumed => 4, + } + } +} + /// A tracked asset lock. Private keys are NOT stored here — they're /// re-derived from funding_type + identity_index via key-wallet's Wallet. #[derive(Debug, Clone)] @@ -86,3 +127,52 @@ impl From<&TrackedAssetLock> for AssetLockEntry { } } } + +#[cfg(test)] +mod tests { + use super::AssetLockStatus; + + /// The lifecycle order the monotonicity guard enforces, written out + /// in full. Pinned by value rather than derived from the enum so a + /// reordering of the declaration (which the FFI discriminants and + /// the SQLite label domain also depend on) cannot silently redefine + /// which writes count as downgrades. + #[test] + fn lifecycle_rank_pins_the_forward_only_order() { + assert_eq!(AssetLockStatus::Built.lifecycle_rank(), 0); + assert_eq!(AssetLockStatus::Broadcast.lifecycle_rank(), 1); + assert_eq!(AssetLockStatus::InstantSendLocked.lifecycle_rank(), 2); + assert_eq!(AssetLockStatus::ChainLocked.lifecycle_rank(), 3); + assert_eq!(AssetLockStatus::Consumed.lifecycle_rank(), 4); + } + + /// Every variant is strictly ordered against every other, and the + /// single comparison the guard actually turns on — a ChainLocked row + /// outranking a late InstantSendLocked write — holds. + #[test] + fn lifecycle_rank_is_strictly_increasing_across_the_lifecycle() { + let lifecycle = [ + AssetLockStatus::Built, + AssetLockStatus::Broadcast, + AssetLockStatus::InstantSendLocked, + AssetLockStatus::ChainLocked, + AssetLockStatus::Consumed, + ]; + for pair in lifecycle.windows(2) { + assert!( + pair[0].lifecycle_rank() < pair[1].lifecycle_rank(), + "{:?} must rank strictly below {:?}", + pair[0], + pair[1] + ); + } + + assert!( + AssetLockStatus::InstantSendLocked.lifecycle_rank() + < AssetLockStatus::ChainLocked.lifecycle_rank(), + "an InstantSendLocked write arriving after a ChainLocked one must \ + be recognizable as a downgrade — this is the comparison the \ + monotonicity guard in `advance_asset_lock_status` turns on" + ); + } +} From 4846410094bc69f4ba839cb38f3f2b6ff2ce87df Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sun, 2 Aug 2026 23:47:32 -0500 Subject: [PATCH 08/12] fix(platform-wallet): serialize wallet lifecycle transitions across generations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remove_wallet` is three separately-locked steps: retire the asset-lock manager, drop the shared `WalletManager` entry, detach the handle from `wallets`. The moment the middle step lands the deterministic `wallet_id` is free, so a concurrent same-mnemonic `register_wallet` legitimately succeeds and publishes a *replacement* generation into `wallets` before the removal reaches its own detach. The unqualified `wallets.remove(wallet_id)` then took that replacement out and returned it as the wallet it had removed, leaving a live wallet registered in `wallet_manager` but absent from `wallets` — invisible to the balance handler and every sync coordinator, and un-removable, because the next `remove_wallet` takes the `WalletNotFound` arm. `AssetLockManager::deactivate` cannot cover this. It is per-*instance*: it makes a handle the caller already holds harmless, but says nothing about which generation owns the map entry, and the replacement's manager is a different instance with its own `status_persist_serial`. Add `PlatformWalletManager::wallet_lifecycle_serial`, held across the whole transition by all three writers of the `wallet_manager` + `wallets` pair — `register_wallet` (insert → publish, excluding the best-effort `identity().sync()` network call), `load_from_persistor` (the whole hydration loop plus its batch rollback), and `remove_wallet` (retire → detach). It is the outermost lock: acquired before `status_persist_serial` and before either map lock, never the reverse, and nothing reachable from inside a transition re-enters one. The detach is additionally generation-checked with `Arc::ptr_eq` against the handle actually retired, so a future path that mutates `wallets` outside a lifecycle transition cannot silently reintroduce the swap. Regression test rendezvous on an arrival signal, never a sleep: with a removal parked between the `WalletManager` drop and the detach, exactly one of two states must be observed — the re-import published a second generation (the bug) or it is queued on `wallet_lifecycle_serial` (the fix). Verified to fail without the register-side acquisition: "a re-import published a replacement generation while a removal was mid-flight". Co-Authored-By: Claude --- .../rs-platform-wallet/src/manager/load.rs | 11 + .../rs-platform-wallet/src/manager/mod.rs | 127 ++++++ .../src/manager/wallet_lifecycle.rs | 392 +++++++++++++++++- 3 files changed, 523 insertions(+), 7 deletions(-) diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index c746fb802b6..40d31680f7b 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -46,6 +46,17 @@ impl PlatformWalletManager

{ let persister_dyn: Arc = Arc::clone(&self.persister) as _; + // Hydration is a lifecycle transition like registration and + // removal: each wallet goes live in `wallet_manager` well before + // it is published into `self.wallets`, and the batch rollback at + // the bottom unwinds both maps. Held across the whole loop so a + // concurrent `remove_wallet` of a deterministic id this batch is + // mid-way through can neither drop a `wallet_manager` entry out + // from under an unpublished wallet nor detach a generation it + // never retired. See + // [`wallet_lifecycle_serial`](PlatformWalletManager::wallet_lifecycle_serial). + let _lifecycle = self.lock_wallet_lifecycle_serial().await; + // Track every wallet successfully inserted into // `wallet_manager` and `self.wallets` during this call so the // batch is transactional: if any later iteration fails (id diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index ade7ac6e0a3..e949f35cff9 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -315,6 +315,24 @@ pub(crate) fn coordinator_worker_config() -> WorkerConfig { } } +/// Test-only rendezvous for +/// [`PlatformWalletManager::remove_pre_detach_gate`]. `arrived` fires +/// once [`remove_wallet`](PlatformWalletManager::remove_wallet) has +/// retired the asset-lock manager and dropped the shared +/// `WalletManager` entry but has NOT yet detached the handle from +/// `wallets`; the removal then blocks on `release`. +/// +/// That is precisely the window in which a same-mnemonic re-import can +/// succeed (the id it collides on is already free) and publish a +/// replacement generation the removal would then detach as if it were +/// the generation it retired. +#[cfg(test)] +#[derive(Clone)] +pub(super) struct RemovePreDetachGate { + pub(super) arrived: Arc, + pub(super) release: Arc, +} + /// Multi-wallet coordinator with SPV sync and event handling. /// /// Events are dispatched through [`PlatformEventManager`] to all registered @@ -399,6 +417,70 @@ pub struct PlatformWalletManager { /// rescan pending" state rather than re-freezing silently on the next /// launch. pub(super) sync_fault: Arc, + /// Serializes whole-wallet **lifecycle transitions** — registration + /// ([`register_wallet`](Self::register_wallet)), hydration + /// ([`load_from_persistor`](Self::load_from_persistor)) and removal + /// ([`remove_wallet`](Self::remove_wallet)) — over the + /// `wallet_manager` + `wallets` pair. + /// + /// Neither of those two locks can do this job. Each transition is a + /// *multi-step* rewrite that takes them one at a time and releases + /// each before taking the next: registration inserts into + /// `wallet_manager`, persists, builds the handle, and only then + /// publishes into `wallets`; removal retires the asset-lock manager, + /// drops the `wallet_manager` entry, and only then detaches from + /// `wallets`. Interleaved, the two produce a torn result even though + /// every individual lock was held correctly. + /// + /// The concrete hazard is a same-mnemonic re-import racing a removal. + /// `wallet_id` is deterministic in (seed, network), so once removal + /// has dropped the `wallet_manager` entry the id is free and a + /// concurrent `register_wallet` legitimately succeeds — publishing a + /// *replacement generation* into `wallets` before the removal reaches + /// its own `wallets.remove(wallet_id)`. That removal then detaches + /// the live replacement (whose `AssetLockManager` was never retired, + /// since `deactivate` ran against the previous generation) and hands + /// it back to the caller as the thing it removed. The manager is left + /// with a wallet registered in `wallet_manager` but absent from + /// `wallets` — invisible to the balance handler and every sync + /// coordinator, and un-removable, because a later `remove_wallet` + /// takes the `WalletNotFound` arm. + /// + /// [`AssetLockManager::deactivate`](crate::AssetLockManager::deactivate) + /// cannot close this: it is per-*instance* by construction, and the + /// replacement's manager is a different instance with its own + /// `status_persist_serial`. Retirement makes a stale handle harmless; + /// it says nothing about which generation owns the map entry. + /// + /// Lock ordering: this is the OUTERMOST lock. Acquire it before + /// `status_persist_serial` (via `deactivate`), before + /// `wallet_manager`, and before `wallets` — never the reverse, and + /// never from code already holding any of them. Nothing reachable + /// from inside a lifecycle transition re-enters one, so no cycle + /// exists. + pub(super) wallet_lifecycle_serial: tokio::sync::Mutex<()>, + /// Test-only gauge of tasks currently BLOCKED on + /// [`wallet_lifecycle_serial`](Self::wallet_lifecycle_serial): + /// incremented before the `lock().await` and RAII-decremented the + /// moment it is acquired (see + /// [`lock_wallet_lifecycle_serial`](Self::lock_wallet_lifecycle_serial)). + /// + /// The arrival signal the lifecycle-ordering test rendezvous on, for + /// the same reason + /// [`status_serial_waiters`](crate::AssetLockManager) exists: a sleep + /// cannot distinguish "the competing registration is queued at the + /// boundary" from "it has not been scheduled yet", so a test that + /// released its parked removal after a delay would grade an + /// unserialized implementation as passing whenever the scheduler + /// happened to run things in the non-regressing order. + #[cfg(test)] + pub(super) wallet_lifecycle_waiters: std::sync::atomic::AtomicUsize, + /// Test-only pause point inside + /// [`remove_wallet`](Self::remove_wallet), between dropping the + /// shared `WalletManager` entry and detaching the handle from + /// `wallets`. `None` (the default) makes the hook a no-op. + #[cfg(test)] + pub(super) remove_pre_detach_gate: std::sync::Mutex>, } impl PlatformWalletManager

{ @@ -519,9 +601,54 @@ impl PlatformWalletManager

{ event_adapter_join: tokio::sync::Mutex::new(Some(event_adapter_join)), registry, sync_fault, + wallet_lifecycle_serial: tokio::sync::Mutex::new(()), + #[cfg(test)] + wallet_lifecycle_waiters: std::sync::atomic::AtomicUsize::new(0), + #[cfg(test)] + remove_pre_detach_gate: std::sync::Mutex::new(None), } } + /// Acquire + /// [`wallet_lifecycle_serial`](Self::wallet_lifecycle_serial). + /// + /// Every lifecycle transition goes through here rather than locking + /// the field directly, so the test-only + /// [`wallet_lifecycle_waiters`](Self::wallet_lifecycle_waiters) gauge + /// sees every arrival at the boundary. In non-test builds this + /// compiles to the bare `lock().await`. + pub(super) async fn lock_wallet_lifecycle_serial(&self) -> tokio::sync::MutexGuard<'_, ()> { + // RAII rather than a bare decrement after the await: if the + // caller's future is dropped while still queued, the count must + // come back down, or a cancelled registration would leave the + // gauge permanently non-zero and every later wait would return + // instantly on a phantom arrival. + #[cfg(test)] + struct WaiterGauge<'a>(&'a std::sync::atomic::AtomicUsize); + #[cfg(test)] + impl Drop for WaiterGauge<'_> { + fn drop(&mut self) { + self.0.fetch_sub(1, std::sync::atomic::Ordering::SeqCst); + } + } + #[cfg(test)] + let waiting = { + self.wallet_lifecycle_waiters + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + WaiterGauge(&self.wallet_lifecycle_waiters) + }; + + let guard = self.wallet_lifecycle_serial.lock().await; + + // Dropped on acquisition, not on release: the gauge answers "who + // is still queued at the boundary", so the holder must not count + // itself. + #[cfg(test)] + drop(waiting); + + guard + } + /// Whether the wallet-event adapter has frozen a durable sync /// watermark this session (dashpay/platform#4069). /// diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index dca3442ed11..f8df80e578c 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -340,6 +340,31 @@ impl PlatformWalletManager

{ wallet.downgrade_to_external_signable(); + // Everything from here to the `self.wallets` publish below is one + // lifecycle transition and must not interleave with another. The + // steps are individually locked but separately so: the wallet is + // live in `wallet_manager` from `insert_wallet` onward, yet + // absent from `wallets` until the very end, and the rollback arms + // in between undo only the former. + // + // A concurrent `remove_wallet` of the SAME deterministic id would + // read that torn state — the id is registered, so it drops the + // `wallet_manager` entry out from under this in-flight + // registration, and its own `wallets.remove` then either misses + // (this registration has not published yet, so the removal + // reports `WalletNotFound` after having already destroyed the + // wallet-manager entry) or detaches the generation this call is + // about to return to its caller. Serializing the whole transition + // is what makes "registered" and "published" the same instant to + // every other lifecycle caller. + // + // Held across `persister.store` and `load_persisted` (both of + // which reenter the host synchronously on iOS) because the + // rollback arms they guard are part of the same transition; + // deliberately NOT across the best-effort `identity().sync()` + // network round-trip at the bottom, which is outside it. + let lifecycle = self.lock_wallet_lifecycle_serial().await; + // Insert into WalletManager. A duplicate (same network-scoped // wallet id already registered) surfaces as the typed // `WalletAlreadyExists` so the create FFI / Swift call sites can @@ -511,11 +536,15 @@ impl PlatformWalletManager

{ let platform_wallet = Arc::new(platform_wallet); - // Register the PlatformWallet handle. + // Register the PlatformWallet handle. This publish closes the + // lifecycle transition: from here the wallet is consistently + // present in both maps, so a queued `remove_wallet` sees a whole + // wallet rather than a half-built one. { let mut wallets = self.wallets.write().await; wallets.insert(wallet_id, Arc::clone(&platform_wallet)); } + drop(lifecycle); // Re-seed the lock-free balance atomic from the wallet's inner // balance now that the wallet is in `self.wallets`. @@ -605,6 +634,30 @@ impl PlatformWalletManager

{ /// `wallet_manager.write()`; calling it under `wallet_manager` would /// invert the documented lock order and deadlock. /// + /// # Why retirement is not enough on its own + /// + /// `deactivate` is per-*instance*. It makes the handle a caller + /// already holds harmless; it says nothing about which generation + /// owns the map entry. The three steps below (retire → drop the + /// `WalletManager` entry → detach from `wallets`) take their locks + /// one at a time, and the moment the middle step completes the + /// deterministic `wallet_id` is free — so a concurrent + /// same-mnemonic `register_wallet` legitimately succeeds and + /// publishes a *replacement* generation into `wallets` before this + /// call reaches its own detach. The unqualified removal that + /// followed then took the replacement out: a live wallet, with a + /// live asset-lock manager, registered in `wallet_manager` but + /// invisible in `wallets` (so no balance update or sync coordinator + /// touches it) and un-removable, because the next `remove_wallet` + /// takes the `WalletNotFound` arm. + /// + /// The whole transition therefore runs under + /// [`wallet_lifecycle_serial`](PlatformWalletManager::wallet_lifecycle_serial), + /// which `register_wallet` and `load_from_persistor` also hold + /// across their own publish spans, and the detach is additionally + /// generation-checked (`Arc::ptr_eq` against the handle actually + /// retired) so no future path can reintroduce the swap silently. + /// /// Idempotency is unchanged: a wallet absent from `self.wallets` still /// returns [`PlatformWalletError::WalletNotFound`] after the shared /// `WalletManager` entry is cleaned up, and `deactivate` is itself a @@ -613,7 +666,20 @@ impl PlatformWalletManager

{ &self, wallet_id: &WalletId, ) -> Result, PlatformWalletError> { - // Retire the asset-lock manager first, holding nothing else. Note + // The whole removal is one lifecycle transition: retire, drop the + // shared `WalletManager` entry, detach from `wallets`. Held from + // before the very first read so no registration of the same + // deterministic id can publish a replacement generation into the + // window this call walks through — see + // [`wallet_lifecycle_serial`](PlatformWalletManager::wallet_lifecycle_serial) + // for why retirement alone cannot cover it. + // + // Taken before `deactivate` (which takes the retired manager's + // `status_persist_serial`) and before either map lock, matching + // the documented outermost-first order. + let _lifecycle = self.lock_wallet_lifecycle_serial().await; + + // Retire the asset-lock manager first, holding no map lock. Note // the read guard is dropped before `deactivate` awaits. let existing = { let wallets = self.wallets.read().await; @@ -650,12 +716,55 @@ impl PlatformWalletManager

{ ids }; - let removed = { - let mut wallets = self.wallets.write().await; - wallets - .remove(wallet_id) - .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(wallet_id)))? + // Test-only pause point: the window a replacement generation + // could be published into. Consumed on arrival so a nested + // removal cannot re-park on it. + #[cfg(test)] + { + let gate = self + .remove_pre_detach_gate + .lock() + .expect("remove pre-detach gate mutex") + .take(); + if let Some(gate) = gate { + gate.arrived.notify_one(); + gate.release.notified().await; + } + } + + // Detach the handle — but only the generation we actually + // retired above. `wallet_lifecycle_serial` already guarantees no + // replacement can have been published since, so a mismatch here + // means that invariant was broken by a future caller mutating + // `wallets` outside a lifecycle transition. Detaching blindly in + // that case is the damaging outcome (a live wallet vanishes from + // the map while staying registered in `wallet_manager`), so + // leave the current entry alone and hand back the generation + // this call retired. + let Some(retired) = existing else { + // Never present in `wallets`. Idempotency contract: report + // `WalletNotFound`, having still cleaned up the shared + // `WalletManager` entry above. + return Err(PlatformWalletError::WalletNotFound(hex::encode(wallet_id))); }; + { + let mut wallets = self.wallets.write().await; + match wallets.get(wallet_id) { + Some(current) if Arc::ptr_eq(current, &retired) => { + wallets.remove(wallet_id); + } + Some(_) => { + tracing::error!( + wallet_id = %hex::encode(wallet_id), + "remove_wallet: a different wallet generation was published \ + under this id mid-removal — leaving it registered rather \ + than detaching a wallet this call never retired" + ); + } + None => {} + } + } + let removed = retired; // Detach the wallet's shielded state from the network // coordinator. After the Phase-2b refactor the coordinator @@ -1246,3 +1355,272 @@ mod retained_asset_lock_manager_tests { retained.deactivate().await; } } + +/// Cross-*generation* lifecycle: a removal and a same-mnemonic +/// re-import must not interleave, so a removal can never detach a +/// replacement wallet it did not retire. +#[cfg(test)] +mod wallet_lifecycle_serialization_tests { + use std::sync::Arc; + use std::time::Duration; + + use dashcore::OutPoint; + use key_wallet::mnemonic::{Language, Mnemonic}; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::wallet::managed_wallet_info::asset_lock_builder::AssetLockFundingType; + use key_wallet::Network; + use tokio::sync::Notify; + + use crate::changeset::{ + ClientStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, + }; + use crate::error::PlatformWalletError; + use crate::events::{EventHandler, PlatformEventHandler}; + use crate::manager::RemovePreDetachGate; + use crate::wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; + use crate::wallet::platform_wallet::WalletId; + use crate::PlatformWalletManager; + + // Canonical all-`abandon` BIP-39 test vector. Deterministic, which is + // the whole point: re-importing it yields the SAME wallet id, so the + // re-import genuinely collides with the removal in flight. + const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ + abandon abandon abandon abandon abandon about"; + + #[derive(Default)] + struct NoopPersister; + + impl PlatformWalletPersistence for NoopPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + Ok(ClientStartState::default()) + } + } + + struct NoopEventHandler; + impl EventHandler for NoopEventHandler {} + impl PlatformEventHandler for NoopEventHandler {} + + /// A synthetic tracked lock — used only to prove the replacement's + /// asset-lock manager is still live, so how it was funded is + /// irrelevant and nothing here broadcasts. + fn tracked_lock(out_point: OutPoint) -> TrackedAssetLock { + TrackedAssetLock { + out_point, + transaction: dashcore::Transaction { + version: 3, + lock_time: 0, + input: vec![], + output: vec![], + special_transaction_payload: None, + }, + account_index: 0, + funding_type: AssetLockFundingType::IdentityRegistration, + identity_index: 0, + amount: 1_000_000, + status: AssetLockStatus::Built, + proof: None, + } + } + + /// Regression: `remove_wallet` must not detach a wallet generation it + /// never retired. + /// + /// `remove_wallet` is three separately-locked steps — retire the + /// asset-lock manager, drop the shared `WalletManager` entry, detach + /// from `wallets`. The moment the middle step lands, the + /// deterministic `wallet_id` is free, so a concurrent same-mnemonic + /// `create_wallet_from_seed_bytes` legitimately succeeds and + /// publishes a *replacement* generation into `wallets`. The + /// unqualified `wallets.remove(wallet_id)` that followed then took + /// the replacement out and returned it as the wallet it had removed, + /// leaving a live wallet registered in `wallet_manager` but absent + /// from `wallets` — invisible to the balance handler and every sync + /// coordinator, and un-removable, because the next `remove_wallet` + /// takes the `WalletNotFound` arm. + /// + /// `AssetLockManager::deactivate` cannot cover this: it is + /// per-instance, and the replacement's manager is a different + /// instance. Retirement makes a stale handle harmless; it says + /// nothing about which generation owns the map entry. + /// + /// The rendezvous is on an arrival signal, never a sleep. With the + /// removal parked in the window, exactly one of two states must be + /// observed: the re-import PUBLISHED a second generation (the + /// unserialized behavior — the bug), or the re-import is QUEUED on + /// `wallet_lifecycle_serial` (the fix). A sleep distinguished + /// neither, since "no replacement yet" could just mean the re-import + /// had not been scheduled. + #[tokio::test] + async fn removal_cannot_detach_a_replacement_registered_mid_removal() { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let event_handler: Arc = Arc::new(NoopEventHandler); + let manager = Arc::new(PlatformWalletManager::new( + sdk, + Arc::new(NoopPersister), + event_handler, + )); + + let network = Network::Testnet; + let mnemonic = + Mnemonic::from_phrase(TEST_MNEMONIC, Language::English).expect("valid test mnemonic"); + let seed_bytes = mnemonic.to_seed(""); + + // `Some(0)` skips the SPV birth-height lookup, so nothing here + // consults SPV or the network. + let original = manager + .create_wallet_from_seed_bytes( + network, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("first create should succeed"); + let wallet_id = original.wallet_id(); + + // 1. Park a removal between the `WalletManager` drop and the + // `wallets` detach — the window in which the id is free. + let arrived = Arc::new(Notify::new()); + let release = Arc::new(Notify::new()); + *manager + .remove_pre_detach_gate + .lock() + .expect("remove pre-detach gate mutex") = Some(RemovePreDetachGate { + arrived: Arc::clone(&arrived), + release: Arc::clone(&release), + }); + + let manager_remover = Arc::clone(&manager); + let remover = tokio::spawn(async move { manager_remover.remove_wallet(&wallet_id).await }); + arrived.notified().await; + + // 2. Re-import the SAME mnemonic while the removal is parked. + // Runs in its own task: with the fix it BLOCKS on + // `wallet_lifecycle_serial` until the removal completes, so + // awaiting it inline would deadlock against the release below. + let manager_reimporter = Arc::clone(&manager); + let reimporter = tokio::spawn(async move { + manager_reimporter + .create_wallet_from_seed_bytes( + network, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + }); + + let mut published_mid_removal = false; + let mut queued = false; + for _ in 0..2_000 { + // A *different* Arc under the same id means a replacement + // generation was published; the original is still mapped at + // this point, so identity — not presence — is the signal. + published_mid_removal = manager + .wallets + .read() + .await + .get(&wallet_id) + .is_some_and(|current| !Arc::ptr_eq(current, &original)); + queued = manager + .wallet_lifecycle_waiters + .load(std::sync::atomic::Ordering::SeqCst) + >= 1; + if published_mid_removal || queued { + break; + } + tokio::time::sleep(Duration::from_millis(5)).await; + } + assert!( + published_mid_removal || queued, + "timed out: the re-import neither published nor reached the \ + lifecycle boundary — the test never exercised the race" + ); + assert!( + !published_mid_removal, + "a re-import published a replacement generation while a removal \ + was mid-flight; the removal's detach would take it back out" + ); + assert!( + queued, + "the re-import must come to rest on wallet_lifecycle_serial while \ + the removal holds it — otherwise the two transitions can still \ + interleave" + ); + + // 3. Release the removal, then let the re-import complete. + release.notify_one(); + let removed = remover + .await + .expect("remover task joined") + .expect("the removal must return the wallet it retired"); + let replacement = reimporter + .await + .expect("reimporter task joined") + .expect("the re-import must succeed once the removal is done"); + + assert!( + Arc::ptr_eq(&removed, &original), + "the removal must hand back the generation it retired, not \ + whatever happened to be mapped when it reached the detach" + ); + assert_eq!( + replacement.wallet_id(), + wallet_id, + "the re-import must reuse the deterministic id — otherwise this \ + test is not exercising the collision the fix is about" + ); + + // The replacement is whole: mapped in `wallets`, registered in + // the shared `WalletManager`, and driving a live asset-lock + // manager. + { + let wallets = manager.wallets.read().await; + let mapped = wallets + .get(&wallet_id) + .expect("the replacement must stay published in `wallets`"); + assert!( + Arc::ptr_eq(mapped, &replacement), + "`wallets` must map the id to the replacement generation" + ); + } + assert!( + manager + .wallet_manager + .read() + .await + .get_wallet_info(&wallet_id) + .is_some(), + "the replacement must stay registered in the shared WalletManager" + ); + replacement + .asset_locks() + .track_asset_lock(tracked_lock(OutPoint::null())) + .await + .expect("the replacement's own asset-lock manager must be live"); + + // And the retired generation stays retired. + assert!( + matches!( + removed + .asset_locks() + .track_asset_lock(tracked_lock(OutPoint::null())) + .await, + Err(PlatformWalletError::AssetLockManagerInactive(_)) + ), + "the removed generation's asset-lock manager must remain retired" + ); + } +} From ec257f8000fe917bce28f763e1851ac108310dd6 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sun, 2 Aug 2026 23:58:49 -0500 Subject: [PATCH 09/12] fix(platform-wallet): a retired manager can no longer build against a replacement wallet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_asset_lock_transaction` went straight to `wallet_manager.write()` with no activity check at all. Wallet ids are deterministic in (seed, network), so deleting a wallet and re-importing the same mnemonic recreates the same id over a fresh `PlatformWalletInfo` and a fresh `AssetLockManager`. A handle retained across that boundary — an `Arc` the FFI still holds and can call `dash_platform_wallet_build_asset_lock_transaction` on — resolves `self.wallet_id` to the replacement generation and would derive a top-up account into it, consume one of its funding addresses, and reserve its UTXOs on behalf of a wallet the user deleted. The consumed index is unrecoverable: these accounts fund OP_RETURN-payload credit outputs that never appear as on-chain UTXOs, so SPV cannot rediscover them. An advisory pre-mutex check does not close it either: it passes, the task parks on `build_persist_serial`, and the removal completes during that park. So the authoritative check has to be taken under the same mutex the retirement must acquire. - `deactivate` now flips `active` while holding `build_persist_serial` and then `status_persist_serial`. It is the only place both are held, and `broadcast_funded_asset_lock` drops the build guard before `track_asset_lock` takes the status one, so no cycle exists. - `build_asset_lock_transaction` splits into a public wrapper that acquires the build guard and a `_locked` variant carrying the authoritative `ensure_active_under_build_serial` check before the wallet lookup. `broadcast_funded_asset_lock` calls `_locked`, since it already holds the guard across build→pool-persist. Two regression tests, both rendezvous'd on the production `build_serial_waiters` gauge rather than a sleep: - a retired handle's build is refused and provably never derives the top-up account or queues a pool snapshot; - a build already queued at the boundary when the retirement lands still refuses. Negative controls: with both checks removed the first test sees a fully-signed transaction spending `m/9'/1'/5'/2'/7'/0`; with only the advisory check restored, the first passes and the second still sees one — so the under-mutex check is what closes the race. Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 278 +++++++++++++++++- .../src/wallet/asset_lock/manager.rs | 148 +++++++++- 2 files changed, 408 insertions(+), 18 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index b1b92de5446..882ce4056a9 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -54,6 +54,13 @@ impl AssetLockManager { /// from `platform-wallet-ffi` — built on top of the /// Keychain-resolver vtable so private keys never cross the FFI /// boundary. + /// + /// Serialized on [`build_persist_serial`] and refuses on a retired + /// manager — see [`build_asset_lock_transaction_locked`] for why the + /// check has to happen under that mutex. + /// + /// [`build_persist_serial`]: AssetLockManager::build_persist_serial + /// [`build_asset_lock_transaction_locked`]: AssetLockManager::build_asset_lock_transaction_locked pub async fn build_asset_lock_transaction( &self, amount_duffs: u64, @@ -61,6 +68,51 @@ impl AssetLockManager { funding_type: AssetLockFundingType, identity_index: u32, signer: &S, + ) -> Result<(Transaction, DerivationPath), PlatformWalletError> { + // Cheap early-out so an obviously-stale handle fails without + // queueing behind an unrelated in-flight build. + self.ensure_active()?; + + let build_serial = self.lock_build_persist_serial().await; + self.build_asset_lock_transaction_locked( + amount_duffs, + account_index, + funding_type, + identity_index, + signer, + &build_serial, + ) + .await + } + + /// [`build_asset_lock_transaction`](Self::build_asset_lock_transaction) + /// for callers that already hold + /// [`build_persist_serial`](Self::build_persist_serial) — namely + /// [`broadcast_funded_asset_lock`](Self::broadcast_funded_asset_lock), + /// which holds it across build→pool-persist so a funding index can + /// never be allocated and lost. Taking it again here would + /// self-deadlock, hence the split. + /// + /// The activity check lives here, under the mutex, rather than in + /// the public wrapper alone. Wallet ids are deterministic in (seed, + /// network), so removing a wallet and re-importing the same mnemonic + /// produces the *same* id over a fresh `PlatformWalletInfo`. A + /// handle retained across that boundary — an `Arc` + /// the FFI still holds, or an operation already parked on an await — + /// resolves `self.wallet_id` to the replacement generation. Without + /// this check it would happily derive a top-up account into it, + /// consume one of its funding addresses, and reserve its UTXOs on + /// behalf of a wallet the user deleted. Because + /// [`deactivate`](Self::deactivate) must take this same mutex to + /// flip the flag, a pass here holds for the whole critical section. + pub(super) async fn build_asset_lock_transaction_locked( + &self, + amount_duffs: u64, + account_index: u32, + funding_type: AssetLockFundingType, + identity_index: u32, + signer: &S, + build_serial: &tokio::sync::MutexGuard<'_, ()>, ) -> Result<(Transaction, DerivationPath), PlatformWalletError> { if amount_duffs == 0 { return Err(PlatformWalletError::AssetLockTransaction( @@ -68,6 +120,11 @@ impl AssetLockManager { )); } + // Authoritative: must precede the `wallet_manager` write lock, + // because everything past it mutates the wallet this id now + // resolves to. + self.ensure_active_under_build_serial(build_serial)?; + let mut wm = self.wallet_manager.write().await; let (wallet, info) = wm .get_wallet_mut_and_info_mut(&self.wallet_id) @@ -619,10 +676,12 @@ impl AssetLockManager { // a dismissed sheet's unstructured task keeps running). // Fail a stale handle before spending a build (which allocates a // funding index and reserves inputs) on a wallet that is gone. - // Advisory only — the authoritative refusal is the same check - // under `status_persist_serial` inside `track_asset_lock` and + // Advisory only — the authoritative refusals are the same check + // under `build_persist_serial` inside + // `build_asset_lock_transaction_locked` and under + // `status_persist_serial` inside `track_asset_lock` and // `promote_built_to_broadcast`, since a removal can land during - // the build or the broadcast await below. + // the queue below, the build, or the broadcast await. self.ensure_active()?; // Test-only occupancy gauge for the serialization gate (see @@ -642,16 +701,20 @@ impl AssetLockManager { .fetch_add(1, std::sync::atomic::Ordering::SeqCst); GateGauge(&self.build_serial_gate) }; - let build_persist_guard = self.build_persist_serial.lock().await; + let build_persist_guard = self.lock_build_persist_serial().await; - // 1. Build the asset lock transaction. + // 1. Build the asset lock transaction. `_locked` because we + // already hold `build_persist_serial`; it re-checks activity + // under that guard, which is the authoritative refusal for a + // removal that landed while we were queued above. let (tx, path) = self - .build_asset_lock_transaction( + .build_asset_lock_transaction_locked( amount_duffs, account_index, funding_type, identity_index, signer, + &build_persist_guard, ) .await?; @@ -3265,4 +3328,207 @@ mod tests { "the refreshed proof must be the durable one" ); } + + /// Whether a per-index `IdentityTopUp` account exists for + /// `identity_index`, on BOTH halves of the wallet. + /// + /// The probe the two regression tests below use, because deriving + /// that account is the FIRST thing a build mutates + /// (`ensure_identity_topup_account`, before any address is consumed + /// or any input reserved). Still absent after a refused build means + /// the build never touched the wallet at all, not that it touched it + /// and rolled back. + async fn topup_account_present( + manager: &AssetLockManager, + identity_index: u32, + ) -> (bool, bool) { + let wm = manager.wallet_manager.read().await; + let (wallet, info) = wm + .get_wallet_and_info(&manager.wallet_id) + .expect("wallet present"); + ( + wallet.accounts.identity_topup.contains_key(&identity_index), + info.core_wallet + .accounts + .identity_topup + .contains_key(&identity_index), + ) + } + + /// Regression: a retired manager must refuse to BUILD, not only to + /// mutate status rows. + /// + /// Wallet ids are deterministic in (seed, network), so deleting a + /// wallet and re-importing the same mnemonic produces the SAME id + /// over a fresh `PlatformWalletInfo` and a fresh `AssetLockManager`. + /// A handle retained across that boundary — an `Arc` + /// the FFI still holds and can call + /// `dash_platform_wallet_build_asset_lock_transaction` on — resolves + /// `self.wallet_id` to the REPLACEMENT generation. `build_asset_lock_ + /// transaction` used to go straight to `wallet_manager.write()` with + /// no activity check at all, so the retired handle would derive a + /// top-up account into the replacement, consume one of its funding + /// addresses, and reserve its UTXOs — on behalf of a wallet the user + /// deleted. The consumed index is durable and unrecoverable: these + /// accounts fund OP_RETURN-payload credit outputs that never appear + /// as on-chain UTXOs, so SPV can never rediscover them. + /// + /// Retirement is what the removal path installs, so this drives it + /// directly. The wallet still resolvable under the id afterwards + /// stands in for the replacement generation — the manager cannot + /// tell the two apart, which is precisely why the flag has to be the + /// gate. + #[tokio::test] + async fn a_retired_manager_refuses_to_build_against_the_wallet_under_its_id() { + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::new(AlwaysOkBroadcaster)).await; + + const TOPUP_INDEX: u32 = 7; + assert_eq!( + topup_account_present(&manager, TOPUP_INDEX).await, + (false, false), + "precondition: the per-index top-up account must not exist yet, \ + or a refused build touching the wallet would be invisible" + ); + let queued_before = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + + // The removal-side retirement. + manager.deactivate().await; + + let built = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityTopUp, + TOPUP_INDEX, + &signer, + ) + .await; + assert!( + matches!(built, Err(PlatformWalletError::AssetLockManagerInactive(_))), + "a build on a retired handle must be refused — the wallet under \ + this id is no longer the one this manager was built for; got {built:?}" + ); + + assert_eq!( + topup_account_present(&manager, TOPUP_INDEX).await, + (false, false), + "the refused build must not have derived a top-up account into \ + the wallet now registered under this id" + ); + assert_eq!( + persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(), + queued_before, + "the refused build must not have queued an address-pool snapshot — \ + a retired manager sharing the persister can overwrite the \ + replacement wallet's durable pool" + ); + } + + /// Regression (the race the flag alone does not close): a build that + /// was already queued at the build→persist boundary when the removal + /// landed must refuse, and the retirement must not cut ahead of a + /// build already past it. + /// + /// An advisory `ensure_active()` before the mutex proves nothing: it + /// passes, the task then parks on `build_persist_serial`, and the + /// removal completes during that park. Only a check taken UNDER the + /// mutex that `deactivate` must itself acquire is authoritative — + /// which is why `deactivate` now holds `build_persist_serial` (then + /// `status_persist_serial`, the only place both are held, and always + /// in that order) while it flips the flag. + /// + /// Interleave, with no sleep standing in for a signal: + /// + /// 1. the test holds `build_persist_serial`, standing in for a build + /// that is past the boundary and mid-flight; + /// 2. `deactivate` is spawned and rendezvous'd on the production + /// `build_serial_waiters` gauge — an arrival observed while the + /// test provably holds the mutex is an arrival that cannot have + /// flipped the flag; + /// 3. a build is spawned and rendezvous'd the same way, so it is + /// provably queued BEHIND the retirement. `tokio::sync::Mutex` is + /// FIFO-fair, so releasing hands the mutex to the retirement + /// first — the ordering this test needs is established by the two + /// rendezvous, not assumed. + #[tokio::test] + async fn a_build_queued_behind_a_retirement_refuses_instead_of_running() { + let (manager, signer, persistence) = + funded_asset_lock_manager(Arc::new(AlwaysOkBroadcaster)).await; + + const TOPUP_INDEX: u32 = 7; + let queued_before = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + + // 1. Stand in for a build holding the boundary. + let in_flight_build = manager.lock_build_persist_serial().await; + + // 2. The retirement must come to rest at the boundary. + let manager_deactivate = Arc::clone(&manager); + let deactivator = tokio::spawn(async move { manager_deactivate.deactivate().await }); + manager.await_build_serial_waiters(1).await; + assert!( + manager.active.load(Ordering::SeqCst), + "`deactivate` must not retire the manager while a build still \ + holds `build_persist_serial` — it would strand a build that has \ + already allocated a funding index with its pool snapshot refused" + ); + + // 3. A build enters after the retirement queued. Its advisory + // pre-check passes (the flag is still set, per the assertion + // above) and it parks — the exact window the bug lived in. + let manager_builder = Arc::clone(&manager); + let builder_signer = signer.clone(); + let builder = tokio::spawn(async move { + manager_builder + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityTopUp, + TOPUP_INDEX, + &builder_signer, + ) + .await + }); + manager.await_build_serial_waiters(2).await; + + // 4. Release; FIFO order gives the mutex to the retirement, then + // the build. + drop(in_flight_build); + deactivator.await.expect("deactivator task joined"); + let built = builder.await.expect("builder task joined"); + + assert!( + matches!(built, Err(PlatformWalletError::AssetLockManagerInactive(_))), + "a build that was queued at the boundary when the removal landed \ + must refuse — resuming it would mutate the replacement wallet a \ + same-mnemonic re-import installs under this same id; got {built:?}" + ); + assert_eq!( + topup_account_present(&manager, TOPUP_INDEX).await, + (false, false), + "the refused build must not have derived a top-up account" + ); + assert_eq!( + persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(), + queued_before, + "the refused build must not have queued anything to the shared \ + persister" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index 357ccce8108..6dd23ed1f37 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -134,6 +134,20 @@ pub struct AssetLockManager { /// yet have collected its pool snapshot. #[cfg(test)] pub(super) build_serial_gate: std::sync::atomic::AtomicUsize, + /// Test-only gauge of tasks currently BLOCKED on + /// [`build_persist_serial`](Self::build_persist_serial): incremented + /// before the `lock().await` and RAII-decremented the moment it is + /// acquired (see + /// [`lock_build_persist_serial`](Self::lock_build_persist_serial)). + /// + /// Distinct from [`build_serial_gate`](Self::build_serial_gate), + /// which counts builds at *or past* the gate for the whole of + /// `broadcast_funded_asset_lock`. This one answers the narrower + /// "who is still queued and cannot get past the boundary while the + /// current holder keeps the lock" — the arrival signal the retirement + /// barrier test rendezvous on, in place of a sleep. + #[cfg(test)] + pub(super) build_serial_waiters: std::sync::atomic::AtomicUsize, /// Serializes every asset-lock **status mutation + persistence /// enqueue** pair, so the order in which rows are mutated in memory /// is the order in which their snapshots reach the persister. @@ -308,6 +322,8 @@ impl AssetLockManager { #[cfg(test)] build_serial_gate: std::sync::atomic::AtomicUsize::new(0), #[cfg(test)] + build_serial_waiters: std::sync::atomic::AtomicUsize::new(0), + #[cfg(test)] resume_pre_promote_gate: std::sync::Mutex::new(None), #[cfg(test)] promote_post_cas_gate: std::sync::Mutex::new(None), @@ -355,6 +371,43 @@ impl AssetLockManager { guard } + /// Acquire [`build_persist_serial`](Self::build_persist_serial). + /// + /// Every build→persist unit goes through here rather than locking + /// the field directly, so the test-only + /// [`build_serial_waiters`](Self::build_serial_waiters) gauge sees + /// every arrival at the boundary. In non-test builds this compiles + /// to the bare `lock().await`. + pub(super) async fn lock_build_persist_serial(&self) -> tokio::sync::MutexGuard<'_, ()> { + // RAII for the same reason as `lock_status_persist_serial`: a + // future dropped while queued must not leave the gauge + // permanently non-zero. + #[cfg(test)] + struct WaiterGauge<'a>(&'a std::sync::atomic::AtomicUsize); + #[cfg(test)] + impl Drop for WaiterGauge<'_> { + fn drop(&mut self) { + self.0.fetch_sub(1, std::sync::atomic::Ordering::SeqCst); + } + } + #[cfg(test)] + let waiting = { + self.build_serial_waiters + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + WaiterGauge(&self.build_serial_waiters) + }; + + let guard = self.build_persist_serial.lock().await; + + // Dropped on acquisition, not on release: the gauge answers + // "who is still queued at the boundary", so the holder must not + // count itself. + #[cfg(test)] + drop(waiting); + + guard + } + /// Retire this manager: no later operation may mutate or persist /// asset-lock state through it. /// @@ -367,27 +420,49 @@ impl AssetLockManager { /// `wallet_id`. See [`active`](Self::active) for why a manager can /// outlive its wallet at all. /// - /// The flag is flipped while HOLDING - /// [`status_persist_serial`](Self::status_persist_serial), which is + /// The flag is flipped while HOLDING **both** ordering mutexes — + /// [`build_persist_serial`](Self::build_persist_serial) and + /// [`status_persist_serial`](Self::status_persist_serial) — which is /// what makes the retirement a barrier rather than a hint: /// - /// - it cannot take the mutex until any in-flight mutate→enqueue - /// unit has released it, so removal never interrupts one halfway - /// (mutated in memory, changeset not yet handed to the persister); + /// - it cannot take either mutex until any in-flight unit has + /// released it, so removal never interrupts one halfway (a build + /// that allocated a funding index but has not persisted the pool; + /// a row mutated in memory whose changeset has not reached the + /// persister); /// - once it returns, every subsequent acquirer — including /// operations that had already started and were parked on - /// `broadcast` / `wait_for_proof` — sees `false` at + /// `broadcast` / `wait_for_proof`, or queued at either boundary — + /// sees `false` at /// [`ensure_active_under_serial`](Self::ensure_active_under_serial) - /// before it touches the wallet row or the persister. + /// or + /// [`ensure_active_under_build_serial`](Self::ensure_active_under_build_serial) + /// before it touches the wallet, a row, or the persister. /// - /// Idempotent: a second call is a no-op (it still takes the mutex, - /// so it still waits out any in-flight unit). + /// Both are needed because the two units are guarded separately and + /// neither subsumes the other: builds mutate `Wallet` / + /// `ManagedWalletInfo` (deriving a top-up account, consuming a + /// funding address, reserving inputs) without ever taking + /// `status_persist_serial`, so retiring under the status mutex alone + /// would leave a queued build free to run against the replacement + /// wallet a same-mnemonic re-import installs under the same + /// deterministic id. + /// + /// Acquisition order is `build_persist_serial` then + /// `status_persist_serial`, and it is the only place both are held: + /// `broadcast_funded_asset_lock` drops the build guard before + /// `track_asset_lock` takes the status one, so nothing ever holds + /// them in the opposite order and no cycle exists. + /// + /// Idempotent: a second call is a no-op (it still takes both + /// mutexes, so it still waits out any in-flight unit). /// /// Must NOT be called while holding the `wallet_manager` lock — the - /// mutate→enqueue units this waits on acquire `wallet_manager` - /// themselves, so doing so would invert the documented - /// `status_persist_serial → wallet_manager` order and deadlock. + /// units this waits on acquire `wallet_manager` themselves, so doing + /// so would invert the documented `serial → wallet_manager` order + /// and deadlock. pub(crate) async fn deactivate(&self) { + let _build = self.lock_build_persist_serial().await; let _serial = self.lock_status_persist_serial().await; let was_active = self.active.swap(false, Ordering::SeqCst); if was_active { @@ -418,6 +493,28 @@ impl AssetLockManager { self.ensure_active() } + /// The authoritative stale-handle check for the *build* unit, taken + /// after acquiring + /// [`build_persist_serial`](Self::build_persist_serial) and BEFORE + /// the `wallet_manager` write lock. + /// + /// Same argument as + /// [`ensure_active_under_serial`](Self::ensure_active_under_serial), + /// against the other mutex: a build mutates the wallet itself + /// (deriving a top-up account, consuming a funding address, + /// reserving inputs) without ever touching a status row, so the + /// status mutex says nothing about it. Because + /// [`deactivate`](Self::deactivate) must take this mutex too, a + /// `true` observed here holds for the rest of the build: either the + /// retirement landed before us and we refuse, or it is queued behind + /// us and the wallet we are about to mutate is still ours. + pub(super) fn ensure_active_under_build_serial( + &self, + _build_serial: &tokio::sync::MutexGuard<'_, ()>, + ) -> Result<(), PlatformWalletError> { + self.ensure_active() + } + /// Cheap advisory activity check for public entry points /// (`resume_asset_lock`, `broadcast_funded_asset_lock`, …), so a /// stale handle fails immediately instead of doing a build or an @@ -471,6 +568,33 @@ impl AssetLockManager { ); } + /// Test-only: wait until at least `n` tasks are blocked on + /// [`build_persist_serial`](Self::build_persist_serial). + /// + /// [`await_status_serial_waiters`](Self::await_status_serial_waiters) + /// for the other ordering mutex; same contract, same reason for + /// polling. + #[cfg(test)] + pub(super) async fn await_build_serial_waiters(&self, n: usize) { + for _ in 0..2_000 { + if self + .build_serial_waiters + .load(std::sync::atomic::Ordering::SeqCst) + >= n + { + return; + } + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + } + panic!( + "timed out waiting for {n} task(s) to block on build_persist_serial \ + (saw {}) — either the competing task never reached the ordering \ + boundary, or the code under test no longer acquires the mutex there", + self.build_serial_waiters + .load(std::sync::atomic::Ordering::SeqCst) + ); + } + /// Queue an `AssetLockChangeSet` onto the per-wallet persister. /// No-op when the changeset is empty. /// From f0367d61b31780ac0fa924a5d0d3903ae22050f4 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Mon, 3 Aug 2026 00:06:19 -0500 Subject: [PATCH 10/12] test(platform-wallet): make the promote/finalize rendezvous unmissable and bounded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `durable_row_after_promote_finalize_interleave` sampled its `queued_before_finalize` baseline AFTER spawning the finalizer. A finalizer that reached its enqueue before the sample folds that store into the baseline, so `> baseline` can never become true — and by then `status_serial_waiters` has fallen back to zero, so neither exit condition can ever fire and the unbounded loop spins until the harness kills the run. The direction that matters: the finalizer is only free to enqueue early when the serialization under test has REGRESSED. So the rendezvous was blind exactly where it had to be sharpest — a regression would surface as a suite timeout rather than as the ordering-inversion assertion. Sampling after the spawn held only because nothing suspends between the two, so the finalizer cannot be polled first. Nothing enforced that invariant, and this file already uses the `multi_thread` flavor elsewhere. - sample the baseline before the spawn; - bound the loop (2000 x 5ms, matching `await_status_serial_waiters`) and panic with a diagnostic rather than spinning; - exit on `finalizer.is_finished()` too, so a finalizer that panicked before reaching the mutex surfaces its real cause at the join instead of leaving both gauges reading like "not scheduled yet" forever; - assert after the joins that one of the two signals actually fired, so a finalizer that returned Ok having queued nothing cannot be mistaken for a passing interleave. Verified by regressing the serialization the test guards (dropping `status_persist_serial` before the post-CAS gate) under a multi-thread runtime with a suspension point above the sample: old sampling hangs past 60s; fixed sampling fails in 0.4s on the intended assertion, "the durable row must not regress below the finalized in-memory status". Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 55 ++++++++++++++++--- 1 file changed, 48 insertions(+), 7 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 882ce4056a9..4d8cd61ff3b 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -2486,6 +2486,29 @@ mod tests { core_chain_locked_height: 4016, out_point, }); + + // Sampled BEFORE the spawn, deliberately. A finalizer that got + // all the way to its enqueue before we sampled would fold that + // store into the baseline, after which `> baseline` can never + // become true — and `status_serial_waiters` has already fallen + // back to zero, so neither exit condition can ever fire. The + // rendezvous below would spin forever, reporting a harness + // timeout instead of the ordering inversion it exists to catch — + // and it is precisely when the serialization REGRESSES that the + // finalizer becomes free to enqueue early, so the sampling was + // blind in the one direction it has to be sharpest. + // + // Sampling after the spawn happened to hold only because nothing + // suspends between the two, so the finalizer cannot be polled + // first. Nothing enforced that: one `.await` above the sample — + // or this test moving to the `multi_thread` flavor already used + // elsewhere in this file — reintroduces the hang. + let queued_before_finalize = persistence + .stored + .lock() + .expect("capturing persistence mutex") + .len(); + let manager_finalizer = Arc::clone(&manager); let finalize_proof = chain_proof.clone(); let finalizer = tokio::spawn(async move { @@ -2518,12 +2541,15 @@ mod tests { // mean the finalizer had not been scheduled, so the pre-fix // implementation could enqueue in the non-regressing order after // the release and falsely pass. - let queued_before_finalize = persistence - .stored - .lock() - .expect("capturing persistence mutex") - .len(); - loop { + // + // Bounded, and with a third exit for a finalizer that finished + // without producing either signal — it panicked before reaching + // the mutex, or a future edit made it return early. Either leaves + // both gauges reading exactly like "not scheduled yet" forever; + // breaking here hands the real cause to the joins below instead + // of hanging on a state that can no longer change. + let mut rendezvous = None; + for _ in 0..2_000 { let finalizer_enqueued = persistence .stored .lock() @@ -2534,11 +2560,19 @@ mod tests { .status_serial_waiters .load(std::sync::atomic::Ordering::SeqCst) >= 1; - if finalizer_enqueued || finalizer_blocked { + if finalizer_enqueued || finalizer_blocked || finalizer.is_finished() { + rendezvous = Some(finalizer_enqueued || finalizer_blocked); break; } tokio::time::sleep(Duration::from_millis(5)).await; } + let rendezvous = rendezvous.unwrap_or_else(|| { + panic!( + "timed out waiting for the finalizer to either enqueue or come \ + to rest on status_persist_serial — the promoter is parked \ + holding that mutex, so one of the two must happen" + ) + }); // 3. Release the stale promoter. release.notify_one(); @@ -2550,6 +2584,13 @@ mod tests { .await .expect("finalizer task joined") .expect("finalize must not error"); + assert!( + rendezvous, + "the finalizer ran to completion without ever enqueueing or \ + blocking on status_persist_serial, so this interleave never \ + exercised the ordering it claims to assert — the joins above \ + passed, so it returned Ok having queued nothing" + ); (persistence.durable_asset_lock(&out_point), chain_proof) } From df4d81c14920ea5e0f0b38cee7cebcb31ba96ca7 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Fri, 7 Aug 2026 09:56:59 -0500 Subject: [PATCH 11/12] fix(platform-wallet): gate asset-lock broadcasts on wallet generation Hold each AssetLockManager's WalletGeneration payment guard across the create/resume liveness check and network send so remove_wallet cannot complete while a broadcast is still in flight. Bind rejected/undersized reservation cleanup to the origin generation under one manager-lock hold so a same-id re-import cannot free a replacement ReservationSet when token counters restart. Co-Authored-By: Claude --- .../src/wallet/asset_lock/build.rs | 305 +++++++++++++++++- .../src/wallet/asset_lock/manager.rs | 86 +++++ .../src/wallet/asset_lock/sync/recovery.rs | 46 ++- .../src/wallet/core/broadcast.rs | 1 + .../src/wallet/identity/network/payments.rs | 11 + .../src/wallet/platform_addresses/provider.rs | 1 + .../src/wallet/platform_addresses/transfer.rs | 1 + .../src/wallet/platform_addresses/wallet.rs | 1 + .../wallet/platform_addresses/withdrawal.rs | 1 + .../src/wallet/platform_wallet.rs | 1 + .../src/wallet/reservations.rs | 82 +++-- 11 files changed, 495 insertions(+), 41 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 249a0861215..c3f7cd63c8e 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -883,9 +883,14 @@ impl AssetLockManager { crate::wallet::reservations::ReservedFundingAccount::CoinJoin(account_index) } }; + // Bound to THIS manager's generation: after dropping + // `build_persist_serial`, a remove/re-import can install a + // replacement under the same deterministic id. The release + // refuses to touch a different generation's ReservationSet. crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, &self.wallet_id, + self.generation(), reserved_account, &tx, reservation_token, @@ -968,7 +973,24 @@ impl AssetLockManager { // transaction — so at no point is the row resumable while its // inputs are re-spendable. A `MaybeSent` failure keeps both the // reservation and the resumable row. + // + // The generation lifecycle gate is held across the liveness check + // AND the network send. Without it, `remove_wallet_with_teardown` + // can take the exclusive gate, deactivate this manager, drop the + // wallet, run host teardown, and return while this call is still + // about to enter (or is still inside) `broadcast` — putting a + // transaction on the wire after the host deleted the wallet and its + // recovery material. Same barrier the deferred-payment path uses + // (`WalletGeneration::payment_guard` + `is_current_generation`). + let _lifecycle = self.admit_broadcast().await?; if let Err(e) = self.broadcaster.broadcast(&tx).await { + // Drop the lifecycle gate before rejection cleanup: untrack takes + // `status_persist_serial` then `wallet_manager`, and the release + // is generation-bound under a manager read lock. Holding the + // shared payment gate across those steps would only stall + // teardown without protecting anything the generation check + // does not already cover. + drop(_lifecycle); if matches!(e, crate::broadcaster::BroadcastError::Rejected { .. }) { // `untrack_asset_lock` queues the changeset itself, as one // serialized unit with the in-memory removal. @@ -998,6 +1020,14 @@ impl AssetLockManager { // the inputs must stay reserved exactly like a `MaybeSent` // outcome, or the still-tracked row would be resumable while // its inputs are re-spendable. + // + // The release is generation-bound: `untrack` releases + // `status_persist_serial` before we get here, so a + // remove/re-import can install a replacement generation in + // that window. `release_reservation_after_rejected_broadcast` + // refuses unless `wallet_id` still resolves to THIS manager's + // generation under one manager-lock hold — matching + // `CoreWallet::release_transaction_reservation`. if removed_built_row { let reserved_account = match funding_account { AssetLockFundingAccount::Bip44 { @@ -1015,6 +1045,7 @@ impl AssetLockManager { crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, &self.wallet_id, + self.generation(), reserved_account, &tx, reservation_token, @@ -1024,6 +1055,11 @@ impl AssetLockManager { } return Err(e.into()); } + // Successful send: drop the gate before the post-broadcast promote. + // The transaction is already on the wire; the promote is a local + // mutate→enqueue unit protected by `status_persist_serial` and the + // activity check, not by the network exclusion barrier. + drop(_lifecycle); // 4. Transition to Broadcast and queue the changeset. // @@ -1137,7 +1173,7 @@ mod tests { use crate::wallet::asset_lock::manager::{ AssetLockManager, PromotePostCasGate, ResumePrePromoteGate, }; - use crate::wallet::asset_lock::tracked::AssetLockStatus; + use crate::wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; use crate::wallet::persister::WalletPersister; use crate::wallet::platform_wallet::PlatformWalletInfo; use crate::wallet::platform_wallet::WalletId; @@ -1250,7 +1286,7 @@ mod tests { broadcaster: Arc, persistence: Arc, ) -> (Arc>, WalletSigner) { - let (wallet_manager, wallet_id, _balance, signer) = + let (wallet_manager, wallet_id, generation, signer) = funded_wallet_manager(StandardAccountType::BIP44Account).await; let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); @@ -1258,6 +1294,7 @@ mod tests { sdk, wallet_manager, wallet_id, + generation, Arc::new(Notify::new()), broadcaster, WalletPersister::new(wallet_id, persistence as Arc), @@ -1297,7 +1334,7 @@ mod tests { Arc, ) { let persistence = Arc::new(CapturingPersistence::default()); - let (wallet_manager, wallet_id, _generation, signer) = + let (wallet_manager, wallet_id, generation, signer) = crate::test_support::funded_coinjoin_wallet_manager().await; let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); @@ -1305,6 +1342,7 @@ mod tests { sdk, wallet_manager, wallet_id, + generation, Arc::new(Notify::new()), broadcaster, WalletPersister::new( @@ -1599,7 +1637,7 @@ mod tests { /// would be resumable while its inputs are re-spendable. #[tokio::test] async fn rejected_broadcast_racing_concurrent_resume_keeps_row_and_reservation() { - let (wallet_manager, wallet_id, _balance, signer) = + let (wallet_manager, wallet_id, generation, signer) = funded_wallet_manager(StandardAccountType::BIP44Account).await; let broadcaster = Arc::new(RejectAfterConcurrentResumeBroadcaster { @@ -1612,6 +1650,7 @@ mod tests { sdk, Arc::clone(&wallet_manager), wallet_id, + Arc::clone(&generation), Arc::new(Notify::new()), broadcaster, WalletPersister::new( @@ -1734,7 +1773,7 @@ mod tests { async fn concurrent_invitation_builds_cannot_roll_back_the_used_index_snapshot() { use key_wallet::account::AccountType; - let (wallet_manager, wallet_id, _balance, signer) = + let (wallet_manager, wallet_id, generation, signer) = crate::test_support::funded_wallet_manager_with_outputs( StandardAccountType::BIP44Account, &[10_000_000, 10_000_000], @@ -1753,6 +1792,7 @@ mod tests { sdk, wallet_manager, wallet_id, + Arc::clone(&generation), Arc::new(Notify::new()), Arc::new(AlwaysOkBroadcaster), WalletPersister::new( @@ -2116,7 +2156,7 @@ mod tests { /// guard then preserves the row and its reservation. #[tokio::test] async fn rejected_broadcast_racing_resume_read_before_broadcast_keeps_row_and_reservation() { - let (wallet_manager, wallet_id, _balance, signer) = + let (wallet_manager, wallet_id, generation, signer) = funded_wallet_manager(StandardAccountType::BIP44Account).await; let create_entered = Arc::new(Notify::new()); @@ -2137,6 +2177,7 @@ mod tests { sdk, Arc::clone(&wallet_manager), wallet_id, + Arc::clone(&generation), Arc::new(Notify::new()), Arc::clone(&broadcaster), WalletPersister::new( @@ -2674,7 +2715,7 @@ mod tests { /// `wait_for_funded_asset_lock_proof`. #[tokio::test] async fn create_broadcast_does_not_downgrade_a_row_finalized_during_the_broadcast() { - let (wallet_manager, wallet_id, _balance, signer) = + let (wallet_manager, wallet_id, generation, signer) = funded_wallet_manager(StandardAccountType::BIP44Account).await; let broadcaster = Arc::new(FinalizeDuringBroadcastBroadcaster { @@ -2689,6 +2730,7 @@ mod tests { sdk, Arc::clone(&wallet_manager), wallet_id, + Arc::clone(&generation), Arc::new(Notify::new()), Arc::clone(&broadcaster), WalletPersister::new( @@ -3901,4 +3943,253 @@ mod tests { persister" ); } + + /// Wallet removal must wait for an in-flight create-side asset-lock + /// broadcast that has already passed its liveness check. + /// + /// The retirement barrier only covers `build_persist_serial` / + /// `status_persist_serial`. Without the generation lifecycle gate + /// across `admit_broadcast` + `broadcast`, removal can deactivate the + /// manager, drop the wallet, and return while the create path is still + /// inside the network send — putting a transaction on the wire after + /// host teardown deleted the wallet's recovery material. + #[tokio::test] + async fn create_broadcast_holds_generation_gate_against_teardown() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + struct ParkingBroadcaster { + entered: Arc, + release: Arc, + calls: AtomicUsize, + } + + #[async_trait] + impl TransactionBroadcaster for ParkingBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + self.entered.notify_one(); + self.release.notified().await; + Ok(transaction.txid()) + } + } + + let entered = Arc::new(Notify::new()); + let release = Arc::new(Notify::new()); + let broadcaster = Arc::new(ParkingBroadcaster { + entered: Arc::clone(&entered), + release: Arc::clone(&release), + calls: AtomicUsize::new(0), + }); + let (manager, signer, _persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + + let generation = Arc::clone(manager.generation()); + let manager_create = Arc::clone(&manager); + let create = tokio::spawn(async move { + manager_create + .broadcast_funded_asset_lock( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await + }); + entered.notified().await; + assert_eq!( + broadcaster.calls.load(Ordering::SeqCst), + 1, + "create must have entered the network send" + ); + + let teardown = tokio::spawn(async move { generation.teardown_guard().await }); + // If create forgot to hold the shared gate, exclusive teardown + // would finish while the send is still parked. + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + assert!( + !teardown.is_finished(), + "teardown must block on the create broadcast's generation payment gate" + ); + + release.notify_one(); + create + .await + .expect("create task joined") + .expect("create broadcast should succeed once released"); + let _guard = teardown.await.expect("teardown task joined"); + } + + /// Resume re-broadcast must hold the same generation payment gate as + /// the create path, or wallet removal can complete while a defensive + /// re-broadcast is still in flight. + #[tokio::test] + async fn resume_broadcast_holds_generation_gate_against_teardown() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + struct ParkingBroadcaster { + entered: Arc, + release: Arc, + calls: AtomicUsize, + } + + #[async_trait] + impl TransactionBroadcaster for ParkingBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + self.entered.notify_one(); + self.release.notified().await; + Ok(transaction.txid()) + } + } + + let entered = Arc::new(Notify::new()); + let release = Arc::new(Notify::new()); + let broadcaster = Arc::new(ParkingBroadcaster { + entered: Arc::clone(&entered), + release: Arc::clone(&release), + calls: AtomicUsize::new(0), + }); + let (manager, signer, _persistence) = + funded_asset_lock_manager(Arc::clone(&broadcaster)).await; + + let (tx, _path) = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await + .expect("build"); + let out_point = OutPoint::new(tx.txid(), 0); + manager + .track_asset_lock(TrackedAssetLock { + out_point, + transaction: tx, + account_index: 0, + funding_type: AssetLockFundingType::IdentityRegistration, + identity_index: 0, + amount: 1_000_000, + status: AssetLockStatus::Built, + proof: None, + }) + .await + .expect("track"); + + let generation = Arc::clone(manager.generation()); + let manager_resume = Arc::clone(&manager); + let resume = tokio::spawn(async move { + manager_resume + .resume_asset_lock(&out_point, Some(std::time::Duration::from_millis(10))) + .await + }); + entered.notified().await; + assert_eq!( + broadcaster.calls.load(Ordering::SeqCst), + 1, + "resume must have entered the re-broadcast" + ); + + let teardown = tokio::spawn(async move { generation.teardown_guard().await }); + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + assert!( + !teardown.is_finished(), + "teardown must block on the resume broadcast's generation payment gate" + ); + + release.notify_one(); + let _ = resume.await.expect("resume task joined"); + let _guard = teardown.await.expect("teardown task joined"); + } + + /// Rejected-broadcast cleanup must not free a replacement generation's + /// reservation under the same deterministic wallet id. + /// + /// After untrack releases its ordering mutex, a remove/re-import can + /// install a replacement whose per-account `ReservationSet` restarts + /// its token counter at zero — so the old build's token can equal a + /// live replacement token. The release validates generation identity + /// under one manager-lock hold before touching the ReservationSet. + #[tokio::test] + async fn rejected_cleanup_does_not_release_replacement_generation_reservation() { + use crate::wallet::core::WalletGeneration; + use crate::wallet::reservations::{ + release_reservation_after_rejected_broadcast, ReservedFundingAccount, + }; + use key_wallet::account::account_type::StandardAccountType; + + let (manager, signer, _persistence) = + funded_asset_lock_manager(Arc::new(AlwaysOkBroadcaster)).await; + let origin_generation = Arc::clone(manager.generation()); + let wallet_id = manager.wallet_id; + + let build_serial = manager.lock_build_persist_serial().await; + let (tx, _path, token) = manager + .build_asset_lock_transaction_with_funding_locked( + crate::wallet::asset_lock::build::AssetLockBuildAmount::Exact(1_000_000), + crate::wallet::asset_lock::build::AssetLockFundingAccount::Bip44 { + account_index: 0, + }, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + &build_serial, + ) + .await + .expect("build original"); + drop(build_serial); + let original_token = token.expect("build must stamp a reservation token"); + + // Simulate a same-id re-import: the map entry under `wallet_id` + // now carries a *different* WalletGeneration Arc (replacement + // identity) while still holding the live ReservationSet the + // original build stamped. A generation-blind release would free + // those inputs via the recycled token. + { + let mut wm = manager.wallet_manager.write().await; + let info = wm + .get_wallet_info_mut(&wallet_id) + .expect("wallet still present"); + info.generation = Arc::new(WalletGeneration::new()); + assert!( + !std::sync::Arc::ptr_eq(&info.generation, &origin_generation), + "replacement generation marker must differ from the origin" + ); + } + + release_reservation_after_rejected_broadcast( + &manager.wallet_manager, + &wallet_id, + &origin_generation, + ReservedFundingAccount::Standard(StandardAccountType::BIP44Account, 0), + &tx, + Some(original_token), + ) + .await; + + // Reservation must still be held: a rebuild over the single-UTXO + // wallet fails at input selection instead of reselecting released + // inputs. + let rebuild = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + assert!( + matches!( + rebuild, + Err(PlatformWalletError::CoreInsufficientFunds { .. }) + | Err(PlatformWalletError::AssetLockTransaction(_)) + | Err(PlatformWalletError::NoSpendableInputs { .. }) + ), + "replacement generation's reservation must remain held after a \ + stale-generation cleanup, got {rebuild:?}" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index 6dd23ed1f37..49d2b65308e 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -12,6 +12,7 @@ use tokio::sync::{Notify, RwLock}; use crate::broadcaster::TransactionBroadcaster; use crate::changeset::changeset::AssetLockChangeSet; use crate::error::PlatformWalletError; +use crate::wallet::core::WalletGeneration; use crate::wallet::persister::WalletPersister; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; @@ -84,6 +85,19 @@ pub struct AssetLockManager { pub(super) wallet_manager: Arc>>, /// Identifies which wallet within the manager this manager operates on. pub(super) wallet_id: WalletId, + /// The wallet *generation* this manager was built for. + /// + /// Wallet ids are deterministic in (seed, network), so removing a wallet + /// and re-importing the same mnemonic reuses the same id under a *fresh* + /// [`WalletGeneration`]. This `Arc` is the unforgeable generation + /// identity (`Arc::ptr_eq` against `PlatformWalletInfo.generation`) and + /// also owns that generation's lifecycle gate — held shared across every + /// network broadcast so removal's exclusive teardown cannot interleave + /// between the liveness check and the send. Same object + /// [`CoreWallet`](crate::CoreWallet) and + /// [`PlatformWallet`](crate::PlatformWallet) hold; see + /// [`WalletGeneration`]. + pub(super) generation: Arc, /// Notified on InstantLock / ChainLock events by SpvEventForwarder. /// Used by `wait_for_proof()` and `wait_for_chain_lock()`. pub(super) lock_notify: Arc, @@ -303,6 +317,7 @@ impl AssetLockManager { sdk: Arc, wallet_manager: Arc>>, wallet_id: WalletId, + generation: Arc, lock_notify: Arc, broadcaster: Arc, persister: WalletPersister, @@ -311,6 +326,7 @@ impl AssetLockManager { sdk, wallet_manager, wallet_id, + generation, lock_notify, broadcaster, persister, @@ -332,6 +348,76 @@ impl AssetLockManager { } } + /// This manager's owning [`WalletGeneration`] — generation identity and + /// lifecycle gate. See the field docs on [`Self::generation`]. + pub(crate) fn generation(&self) -> &Arc { + &self.generation + } + + /// Enter this generation's lifecycle gate as a payment/broadcast + /// operation — see [`WalletGeneration::payment_guard`]. + /// + /// Held across the + /// [`is_current_generation`](Self::is_current_generation) check and + /// every network send so removal cannot interleave between them and + /// complete while an asset-lock transaction is still about to hit the + /// wire. Lock order: this BEFORE `wallet_manager` (and before either + /// ordering mutex that is itself taken before `wallet_manager`). + pub(super) async fn generation_payment_guard(&self) -> tokio::sync::RwLockReadGuard<'_, ()> { + self.generation.payment_guard().await + } + + /// Whether the generation this manager names is still the one + /// registered under `wallet_id` in the shared `WalletManager`. + /// + /// Same identity as + /// [`CoreWallet::is_current_generation`](crate::CoreWallet::is_current_generation): + /// `Arc::ptr_eq` on the per-generation marker. A removed generation + /// (or a same-id replacement) returns `false`. + /// + /// Callers that act on a `true` result must already hold + /// [`generation_payment_guard`](Self::generation_payment_guard) — + /// without it a removal can land between the check and the action. + pub(super) async fn is_current_generation(&self) -> bool { + let wm = self.wallet_manager.read().await; + wm.get_wallet_info(&self.wallet_id) + .is_some_and(|info| Arc::ptr_eq(&info.generation, &self.generation)) + } + + /// Authoritative pre-broadcast admission: take this generation's + /// shared lifecycle gate, then refuse if the generation is no longer + /// current (or the manager has been retired). + /// + /// The returned guard MUST be held for the entire subsequent + /// `broadcaster.broadcast(...)` call. Dropping it re-opens the + /// window for removal's exclusive teardown to complete while the + /// transaction is still on the wire. + /// + /// Returns [`PlatformWalletError::AssetLockManagerInactive`] when the + /// generation is gone or the manager was deactivated — the same + /// stale-handle signal the mutate→enqueue paths use, so FFI hosts + /// re-acquire from the current wallet. + pub(super) async fn admit_broadcast( + &self, + ) -> Result, PlatformWalletError> { + // Gate first, then the manager read for the generation check. + // Teardown takes the exclusive side and then the manager write + // lock, so this order cannot deadlock against removal. + let lifecycle = self.generation_payment_guard().await; + if !self.is_current_generation().await { + return Err(PlatformWalletError::AssetLockManagerInactive(hex::encode( + self.wallet_id, + ))); + } + // Defense in depth: a manager can be deactivated without the map + // entry vanishing only if a future path retires it in place. The + // production remover always deactivates under the exclusive gate + // it already holds, so a live generation implies active — but the + // check is cheap and keeps the inactive signal uniform. + self.ensure_active()?; + Ok(lifecycle) + } + /// Acquire [`status_persist_serial`](Self::status_persist_serial). /// /// Every async mutate→enqueue pair goes through here rather than diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index 97f75667675..1c37cd41272 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -375,8 +375,17 @@ impl AssetLockManager { AssetLockStatus::Built => { // Promoted to `Broadcast` in step 1b — this arm owns that // promotion, so it is the one that re-broadcasts. + // + // Hold the generation lifecycle gate across the liveness + // check and the network send so wallet removal cannot + // complete while this re-broadcast is still in flight + // (host teardown would otherwise delete recovery material + // for a transaction that can still reach the network). + let _lifecycle = self.admit_broadcast().await?; match self.broadcaster.broadcast(&tx).await { - Ok(_) => {} + Ok(_) => { + drop(_lifecycle); + } Err(e @ BroadcastError::Rejected { .. }) => { // Keep `Broadcast`: a concurrent successful resume // may own that status, and the `Broadcast` arm @@ -415,15 +424,22 @@ impl AssetLockManager { // rather than failing the resume on a tx that is actually // fine. If the tx really was mined, `wait_for_proof` // resolves immediately from the SPV/persisted record. - if let Err(e) = self.broadcaster.broadcast(&tx).await { - tracing::debug!( - outpoint = %out_point, - error = %e, - "resume_asset_lock: defensive re-broadcast of a \ - Broadcast-status lock returned an error (likely \ - already in a mempool or mined); proceeding to wait \ - for proof" - ); + // + // Same generation barrier as the `Built` arm: a defensive + // re-broadcast is still a network send for this generation + // and must not race wallet teardown. + { + let _lifecycle = self.admit_broadcast().await?; + if let Err(e) = self.broadcaster.broadcast(&tx).await { + tracing::debug!( + outpoint = %out_point, + error = %e, + "resume_asset_lock: defensive re-broadcast of a \ + Broadcast-status lock returned an error (likely \ + already in a mempool or mined); proceeding to wait \ + for proof" + ); + } } let proof = self.wait_for_proof(out_point, timeout).await?; self.validate_or_upgrade_proof(proof, account_index, out_point) @@ -659,7 +675,7 @@ mod tests { #[tokio::test] async fn built_resume_rebroadcasts_original_and_typed_failures_do_not_broadcast() { - let (wallet_manager, wallet_id, _balance, signer) = + let (wallet_manager, wallet_id, generation, signer) = funded_wallet_manager(StandardAccountType::BIP44Account).await; let persistence = Arc::new(RecordingPersistence::default()); let broadcaster = Arc::new(RecordingBroadcaster::default()); @@ -673,6 +689,7 @@ mod tests { sdk, Arc::clone(&wallet_manager), wallet_id, + Arc::clone(&generation), Arc::new(Notify::new()), Arc::clone(&broadcaster), WalletPersister::new(wallet_id, persistence), @@ -795,7 +812,7 @@ mod tests { // --- Session 1: build a top-up asset lock. This lazily creates // the IdentityTopUp{7} account and must persist its registration. - let (wallet_manager, wallet_id, _balance, signer) = + let (wallet_manager, wallet_id, generation, signer) = funded_wallet_manager(StandardAccountType::BIP44Account).await; let persistence = Arc::new(RecordingPersistence::default()); // The mock SDK network must match the testnet wallet fixture: @@ -811,6 +828,7 @@ mod tests { Arc::clone(&sdk), wallet_manager, wallet_id, + Arc::clone(&generation), Arc::new(Notify::new()), Arc::new(AlwaysRejectedBroadcaster), WalletPersister::new( @@ -872,9 +890,10 @@ mod tests { accounts.insert(account).expect("insert restored account"); } let restored_wallet = Wallet::new_external_signable(Network::Testnet, wallet_id, accounts); + let generation = Arc::new(WalletGeneration::new()); let mut restored_info = PlatformWalletInfo { core_wallet: ManagedWalletInfo::from_wallet(&restored_wallet, 0), - generation: Arc::new(WalletGeneration::new()), + generation: Arc::clone(&generation), identity_manager: IdentityManager::new(), tracked_asset_locks: BTreeMap::new(), }; @@ -906,6 +925,7 @@ mod tests { sdk, Arc::new(RwLock::new(wm)), wallet_id, + generation, Arc::new(Notify::new()), Arc::new(AlwaysRejectedBroadcaster), WalletPersister::new(wallet_id, persistence as Arc), diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 0176d661d3a..caab2089c27 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -90,6 +90,7 @@ impl CoreWallet { self.broadcaster.as_ref(), &self.wallet_manager, &self.wallet_id, + self.generation(), account_type, account_index, transaction, 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 f2f65ace31f..a7e0f59b254 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -1291,10 +1291,21 @@ impl DashPayView<'_, B> { // --- 3. Broadcast the transaction, releasing the build's UTXO // reservation if the broadcast is definitively rejected pre-send. --- + // DashPay payments ride the identity wallet's shared manager Arc; the + // funding reservation was minted against the *wallet generation* that + // owns this identity. Resolve it under a read lock so a remove/re-import + // under the same deterministic id cannot free a replacement's inputs. + let origin_generation = { + let wm = self.wallet_manager.read().await; + wm.get_wallet_info(&self.wallet_id) + .map(|info| Arc::clone(&info.generation)) + .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))? + }; let txid = match crate::wallet::reservations::broadcast_releasing_on_rejection( self.broadcaster.as_ref(), &self.wallet_manager, &self.wallet_id, + &origin_generation, key_wallet::account::account_type::StandardAccountType::BIP44Account, 0, &tx, diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs index b141af762ae..2de793bdb8d 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs @@ -1681,6 +1681,7 @@ mod tests { Arc::clone(&sdk), Arc::clone(&wallet_manager), WALLET, + Arc::new(crate::wallet::core::WalletGeneration::new()), Arc::new(Notify::new()), broadcaster, persister.clone(), diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs index 38db2fd492f..0652a6261b4 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs @@ -1937,6 +1937,7 @@ mod auto_select_tests { Arc::clone(&sdk), Arc::clone(&wallet_manager), [0u8; 32], + Arc::new(crate::wallet::core::WalletGeneration::new()), Arc::new(Notify::new()), broadcaster, persister.clone(), diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 1d9c7b26e63..38c2ae6fba6 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -778,6 +778,7 @@ mod tests { Arc::clone(&sdk), Arc::clone(&wallet_manager), [0u8; 32], + Arc::new(crate::wallet::core::WalletGeneration::new()), Arc::new(Notify::new()), broadcaster, persister.clone(), diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs index e1fef220d44..8b0961da0a8 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs @@ -1570,6 +1570,7 @@ mod plan_withdrawal_seam_tests { Arc::clone(&sdk), Arc::clone(&wallet_manager), wallet_id, + Arc::new(crate::wallet::core::WalletGeneration::new()), Arc::new(Notify::new()), broadcaster, persister.clone(), diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index bf28a640c2d..b9ef24ab571 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -459,6 +459,7 @@ impl PlatformWallet { Arc::clone(&sdk), Arc::clone(&wallet_manager), wallet_id, + Arc::clone(&generation), lock_notify, broadcaster, wallet_persister.clone(), diff --git a/packages/rs-platform-wallet/src/wallet/reservations.rs b/packages/rs-platform-wallet/src/wallet/reservations.rs index 365d6514229..bd4804e40f0 100644 --- a/packages/rs-platform-wallet/src/wallet/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/reservations.rs @@ -14,12 +14,15 @@ //! `Built` row first); those call the broadcaster directly and then //! [`release_reservation_after_rejected_broadcast`]. +use std::sync::Arc; + use dashcore::{Transaction, Txid}; use key_wallet::account::account_type::StandardAccountType; use key_wallet_manager::WalletManager; use tokio::sync::RwLock; use crate::broadcaster::{BroadcastError, TransactionBroadcaster}; +use crate::wallet::core::WalletGeneration; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; /// Broadcast `tx` and reconcile the funding account's UTXO reservation on @@ -43,6 +46,7 @@ pub(crate) async fn broadcast_releasing_on_rejection>, wallet_id: &WalletId, + origin_generation: &Arc, account_type: StandardAccountType, account_index: u32, tx: &Transaction, @@ -54,6 +58,7 @@ pub(crate) async fn broadcast_releasing_on_rejection>, wallet_id: &WalletId, + origin_generation: &Arc, funding_account: ReservedFundingAccount, tx: &Transaction, reservation_token: Option, ) { - // `release_reservation` takes `&self` and the manager map is - // untouched, so a read lock suffices — this cleanup does not - // serialize concurrent sends. + // Generation validation AND the ReservationSet mutation under one + // manager-lock hold. A recreate needs the manager *write* lock, so it + // cannot interleave between the pointer check and the release below. let wm = wallet_manager.read().await; - let account = wm - .get_wallet_and_info(wallet_id) - .and_then(|(_, info)| match funding_account { - ReservedFundingAccount::Standard(StandardAccountType::BIP44Account, account_index) => { - info.core_wallet - .bip44_managed_account_at_index(account_index) - } - ReservedFundingAccount::Standard(StandardAccountType::BIP32Account, account_index) => { - info.core_wallet - .bip32_managed_account_at_index(account_index) - } - ReservedFundingAccount::CoinJoin(account_index) => info - .core_wallet - .accounts - .coinjoin_accounts - .get(&account_index), - }); + let Some((_, info)) = wm.get_wallet_and_info(wallet_id) else { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + ?funding_account, + "could not release UTXO reservation after rejected broadcast: \ + wallet not found" + ); + return; + }; + if !Arc::ptr_eq(&info.generation, origin_generation) { + // The wallet under this id is a different (re-created) generation: + // releasing by outpoint / token could free ITS reservation. Leave + // it — the original generation's reservation ceased to exist with + // it. + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + ?funding_account, + "skipping reservation release after rejected broadcast: wallet was \ + re-created under the same id (different generation) since the \ + reservation was minted" + ); + return; + } + let account = match funding_account { + ReservedFundingAccount::Standard(StandardAccountType::BIP44Account, account_index) => info + .core_wallet + .bip44_managed_account_at_index(account_index), + ReservedFundingAccount::Standard(StandardAccountType::BIP32Account, account_index) => info + .core_wallet + .bip32_managed_account_at_index(account_index), + ReservedFundingAccount::CoinJoin(account_index) => info + .core_wallet + .accounts + .coinjoin_accounts + .get(&account_index), + }; match account { // Owner-guarded when the build's `ReservationToken` is available: // this cleanup always runs after `.await`s (build → broadcast), so @@ -132,7 +172,7 @@ pub(crate) async fn release_reservation_after_rejected_broadcast( wallet_id = %hex::encode(wallet_id), ?funding_account, "could not release UTXO reservation after rejected broadcast: \ - wallet or funds account not found" + funds account not found" ), } } From 572ff0b035ef6a0df5937de295373b8abdaa26f9 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Fri, 7 Aug 2026 09:57:10 -0500 Subject: [PATCH 12/12] fix(platform-wallet): map inactive manager and correct lifecycle docs Surface AssetLockManagerInactive as NotFound at the FFI boundary so hosts can re-acquire without parsing English messages. Document wallet_lifecycle as registration/hydration only and drop the unused waiter instrumentation that no longer matches generation-aware removal. Co-Authored-By: Claude --- packages/rs-platform-wallet-ffi/src/error.rs | 34 +++++ .../rs-platform-wallet/src/manager/load.rs | 16 +-- .../rs-platform-wallet/src/manager/mod.rs | 127 +++++------------- 3 files changed, 74 insertions(+), 103 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index d27c5a822ec..983c0ee7f71 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -468,6 +468,14 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::AssetLockFundingMismatch { .. } => { PlatformWalletFFIResultCode::ErrorAssetLockFundingMismatch } + // A retained asset-lock manager whose wallet generation was removed + // (or replaced under the same deterministic id). The owning generation + // no longer exists, so the same NotFound semantic as a missing wallet + // handle applies — Swift/Kotlin re-acquire from the current wallet + // rather than parsing the English message. + PlatformWalletError::AssetLockManagerInactive(..) => { + PlatformWalletFFIResultCode::NotFound + } // A quiesce/drain barrier that did not complete within budget // (clear/reset paths). The host must fail closed: keep its // callback context alive and skip any paired persistence wipe. @@ -1042,6 +1050,32 @@ mod tests { ); } + /// A retained asset-lock manager whose wallet was removed must surface + /// as NotFound (the generation no longer exists), not ErrorUnknown — + /// Swift/Kotlin re-acquire from the current wallet on that code. + #[test] + fn asset_lock_manager_inactive_maps_to_not_found() { + let err = PlatformWalletError::AssetLockManagerInactive("deadbeef".to_string()); + let rendered = err.to_string(); + let result: PlatformWalletFFIResult = err.into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::NotFound, + "AssetLockManagerInactive should map to NotFound (rendered: {rendered})" + ); + let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_string_lossy() + .into_owned(); + assert_eq!( + msg, rendered, + "Display payload must survive the FFI boundary verbatim" + ); + assert!( + msg.contains("no longer active"), + "typed Display must name the inactive condition: {msg}" + ); + } + /// Other wallet-error variants without a dedicated FFI arm still /// fall through to `ErrorUnknown` while carrying the typed /// Display rendering as the message. Pin this so the catch-all diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index 5b9ff23c8dc..73a946226f4 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -46,14 +46,14 @@ impl PlatformWalletManager

{ let persister_dyn: Arc = Arc::clone(&self.persister) as _; - // Hydration is a lifecycle transition like registration and - // removal: each wallet goes live in `wallet_manager` well before - // it is published into `self.wallets`, and the batch rollback at - // the bottom unwinds both maps. Held across the whole loop so a - // concurrent `remove_wallet` of a deterministic id this batch is - // mid-way through can neither drop a `wallet_manager` entry out - // from under an unpublished wallet nor detach a generation it - // never retired. See + // Hydration is a multi-step registration-class rewrite: each wallet + // goes live in `wallet_manager` well before it is published into + // `self.wallets`, and the batch rollback at the bottom unwinds + // both maps. Held across the whole loop so a concurrent + // registration/hydration of a deterministic id this batch is + // mid-way through cannot interleave those steps. Removal is + // generation-gated separately and does not take this mutex — + // see // [`wallet_lifecycle_serial`](PlatformWalletManager::wallet_lifecycle_serial). let _lifecycle = self.lock_wallet_lifecycle_serial().await; diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index 28f08584b53..bfd5f5f0178 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -418,68 +418,39 @@ pub struct PlatformWalletManager { /// failed / rescan pending" state rather than re-freezing silently on /// the next launch. pub(super) sync_fault: Arc, - /// Serializes whole-wallet **lifecycle transitions** — registration - /// ([`register_wallet`](Self::register_wallet)), hydration - /// ([`load_from_persistor`](Self::load_from_persistor)) and removal - /// ([`remove_wallet`](Self::remove_wallet)) — over the - /// `wallet_manager` + `wallets` pair. + /// Serializes whole-wallet **registration and hydration** — + /// [`register_wallet`](Self::register_wallet) / + /// [`create_wallet_from_seed_bytes`](Self::create_wallet_from_seed_bytes) + /// and [`load_from_persistor`](Self::load_from_persistor) — over the + /// multi-step `wallet_manager` + `wallets` rewrite those paths perform. /// - /// Neither of those two locks can do this job. Each transition is a - /// *multi-step* rewrite that takes them one at a time and releases - /// each before taking the next: registration inserts into - /// `wallet_manager`, persists, builds the handle, and only then - /// publishes into `wallets`; removal retires the asset-lock manager, - /// drops the `wallet_manager` entry, and only then detaches from - /// `wallets`. Interleaved, the two produce a torn result even though - /// every individual lock was held correctly. + /// Registration inserts into `wallet_manager`, persists, builds the + /// handle, and only then publishes into `wallets`. Hydration walks the + /// same maps in the opposite direction. Without an outer serial, two + /// concurrent creates (or a create racing a load) can interleave those + /// steps even though every individual map lock was held correctly. /// - /// The concrete hazard is a same-mnemonic re-import racing a removal. - /// `wallet_id` is deterministic in (seed, network), so once removal - /// has dropped the `wallet_manager` entry the id is free and a - /// concurrent `register_wallet` legitimately succeeds — publishing a - /// *replacement generation* into `wallets` before the removal reaches - /// its own `wallets.remove(wallet_id)`. That removal then detaches - /// the live replacement (whose `AssetLockManager` was never retired, - /// since `deactivate` ran against the previous generation) and hands - /// it back to the caller as the thing it removed. The manager is left - /// with a wallet registered in `wallet_manager` but absent from - /// `wallets` — invisible to the balance handler and every sync - /// coordinator, and un-removable, because a later `remove_wallet` - /// takes the `WalletNotFound` arm. + /// Removal does **not** take this mutex. Same-id re-import is allowed + /// in the free-id window after a removal has dropped the inner + /// `WalletManager` entry; the replacement is protected by + /// generation-aware detach (`Arc::ptr_eq` on the retired generation) + /// and by each generation's own lifecycle gate + /// ([`WalletGeneration::teardown_guard`](crate::wallet::core::WalletGeneration::teardown_guard)), + /// not by serializing registration against removal. See + /// [`remove_wallet_with_teardown`](Self::remove_wallet_with_teardown). /// - /// [`AssetLockManager::deactivate`](crate::AssetLockManager::deactivate) - /// cannot close this: it is per-*instance* by construction, and the - /// replacement's manager is a different instance with its own - /// `status_persist_serial`. Retirement makes a stale handle harmless; - /// it says nothing about which generation owns the map entry. - /// - /// Lock ordering: this is the OUTERMOST lock. Acquire it before - /// `status_persist_serial` (via `deactivate`), before - /// `wallet_manager`, and before `wallets` — never the reverse, and - /// never from code already holding any of them. Nothing reachable - /// from inside a lifecycle transition re-enters one, so no cycle - /// exists. + /// Lock ordering: when held, this is the OUTERMOST lock of a + /// registration/hydration transition. Acquire it before + /// `wallet_manager` and before `wallets` — never the reverse, and + /// never from code already holding either. Nothing reachable from + /// inside a registration/hydration transition re-enters one, so no + /// cycle exists. pub(super) wallet_lifecycle_serial: tokio::sync::Mutex<()>, - /// Test-only gauge of tasks currently BLOCKED on - /// [`wallet_lifecycle_serial`](Self::wallet_lifecycle_serial): - /// incremented before the `lock().await` and RAII-decremented the - /// moment it is acquired (see - /// [`lock_wallet_lifecycle_serial`](Self::lock_wallet_lifecycle_serial)). - /// - /// The arrival signal the lifecycle-ordering test rendezvous on, for - /// the same reason - /// [`status_serial_waiters`](crate::AssetLockManager) exists: a sleep - /// cannot distinguish "the competing registration is queued at the - /// boundary" from "it has not been scheduled yet", so a test that - /// released its parked removal after a delay would grade an - /// unserialized implementation as passing whenever the scheduler - /// happened to run things in the non-regressing order. - #[cfg(test)] - pub(super) wallet_lifecycle_waiters: std::sync::atomic::AtomicUsize, /// Test-only pause point inside - /// [`remove_wallet`](Self::remove_wallet), between dropping the - /// shared `WalletManager` entry and detaching the handle from - /// `wallets`. `None` (the default) makes the hook a no-op. + /// [`remove_wallet_with_teardown`](Self::remove_wallet_with_teardown), + /// between dropping the shared `WalletManager` entry and detaching + /// the handle from `wallets`. `None` (the default) makes the hook a + /// no-op. Used by the generation-aware detach regression test. #[cfg(test)] pub(super) remove_pre_detach_gate: std::sync::Mutex>, } @@ -607,50 +578,16 @@ impl PlatformWalletManager

{ sync_fault, wallet_lifecycle_serial: tokio::sync::Mutex::new(()), #[cfg(test)] - wallet_lifecycle_waiters: std::sync::atomic::AtomicUsize::new(0), - #[cfg(test)] remove_pre_detach_gate: std::sync::Mutex::new(None), } } /// Acquire - /// [`wallet_lifecycle_serial`](Self::wallet_lifecycle_serial). - /// - /// Every lifecycle transition goes through here rather than locking - /// the field directly, so the test-only - /// [`wallet_lifecycle_waiters`](Self::wallet_lifecycle_waiters) gauge - /// sees every arrival at the boundary. In non-test builds this - /// compiles to the bare `lock().await`. + /// [`wallet_lifecycle_serial`](Self::wallet_lifecycle_serial) for a + /// registration or hydration transition. Removal does not take this + /// lock — see the field docs. pub(super) async fn lock_wallet_lifecycle_serial(&self) -> tokio::sync::MutexGuard<'_, ()> { - // RAII rather than a bare decrement after the await: if the - // caller's future is dropped while still queued, the count must - // come back down, or a cancelled registration would leave the - // gauge permanently non-zero and every later wait would return - // instantly on a phantom arrival. - #[cfg(test)] - struct WaiterGauge<'a>(&'a std::sync::atomic::AtomicUsize); - #[cfg(test)] - impl Drop for WaiterGauge<'_> { - fn drop(&mut self) { - self.0.fetch_sub(1, std::sync::atomic::Ordering::SeqCst); - } - } - #[cfg(test)] - let waiting = { - self.wallet_lifecycle_waiters - .fetch_add(1, std::sync::atomic::Ordering::SeqCst); - WaiterGauge(&self.wallet_lifecycle_waiters) - }; - - let guard = self.wallet_lifecycle_serial.lock().await; - - // Dropped on acquisition, not on release: the gauge answers "who - // is still queued at the boundary", so the holder must not count - // itself. - #[cfg(test)] - drop(waiting); - - guard + self.wallet_lifecycle_serial.lock().await } /// Whether the wallet-event adapter has frozen a durable sync