From 3fd964406577f329598856bf4bc0b371a00689ff Mon Sep 17 00:00:00 2001 From: HashEngineering Date: Thu, 6 Aug 2026 15:23:02 -0700 Subject: [PATCH] fix(platform-wallet): settle already-consumed locks and survive an ambiguous resume broadcast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a completed or in-flight asset-lock top-up could never finish, both observed on Android testnet: **Platform's "already completely used" verdict was dropped.** When a transition is rejected with IdentityAssetLockTransactionOutPointAlreadyConsumedError, the credits it would have bought already landed — an earlier attempt succeeded and the client never learned. Nothing recorded that locally: consume_asset_lock was only ever called on the success path, so the lock stayed in the resumable set and every recovery pass re-submitted it for the same deterministic rejection. Clients that block new funding while a lock is unresolved could not buy credits at all until they special-cased the error string. Both funded flows now classify that rejection (typed, via the consensus error rather than its message), mark the lock Consumed, and return the same AssetLockAlreadyConsumed a resume of a consumed lock already raises — so callers need one terminal case, not a string match. **A Built-status resume aborted on an ambiguous re-broadcast.** The Built arm propagated every broadcast error, including MaybeSent. For a lock stuck at Built whose transaction WAS broadcast (the app died between the send and the status advance), MaybeSent is the expected answer on every retry — DAPI classifies all failures that way — so the resume failed, the lock stayed Built, and the next pass repeated it. The top-up never completed. Only a definite Rejected now stops the resume; MaybeSent advances to Broadcast and proceeds to the proof wait, matching what the Broadcast arm already does with the identical signal and keeping a genuinely un-broadcast tx resumable at Built. Tests: two regression tests covering the ambiguous and definite branches (status transition asserted, not just the error). cargo test -p platform-wallet --lib asset_lock:: — 26 passed. cargo clippy -p platform-wallet --lib --tests — no new warnings (the 3 reported are present on the unmodified base). Co-Authored-By: Claude Fable 5 --- packages/rs-platform-wallet/src/error.rs | 41 +++++ .../src/wallet/asset_lock/sync/recovery.rs | 142 +++++++++++++++++- .../wallet/identity/network/registration.rs | 64 +++++++- 3 files changed, 241 insertions(+), 6 deletions(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 9d6ce8a0a4e..503ca22b707 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -514,6 +514,47 @@ pub fn is_instant_lock_proof_invalid(error: &dash_sdk::Error) -> bool { ) } +/// Extract the outpoint from Platform's rejection of an asset lock whose +/// credit output has already been spent by an earlier transition +/// (`IdentityAssetLockTransactionOutPointAlreadyConsumedError`, "Asset lock +/// transaction {txid} output {n} already completely used"). +/// +/// The rejection is **deterministic and terminal**: the credits it would have +/// bought already landed, so every retry receives the same answer. Callers +/// treat it as the success-shaped outcome it really is — mark the lock +/// [`Consumed`](crate::wallet::asset_lock::tracked::AssetLockStatus::Consumed) +/// and stop resuming it — rather than as a failure to retry. +/// +/// Returns the outpoint Platform named, which identifies the local tracked +/// lock without trusting the caller's bookkeeping. +/// +/// Companion to [`is_instant_lock_proof_invalid`]: both classify a Platform +/// rejection of an asset-lock proof, but that one is retryable via a CL +/// upgrade while this one can never succeed. +pub fn asset_lock_already_consumed_out_point( + error: &dash_sdk::Error, +) -> Option { + use dpp::consensus::basic::BasicError; + use dpp::consensus::ConsensusError; + + let consensus_error = match error { + dash_sdk::Error::StateTransitionBroadcastError(broadcast_err) => { + broadcast_err.cause.as_ref() + } + dash_sdk::Error::Protocol(dpp::ProtocolError::ConsensusError(ce)) => Some(ce.as_ref()), + _ => None, + }; + match consensus_error { + Some(ConsensusError::BasicError( + BasicError::IdentityAssetLockTransactionOutPointAlreadyConsumedError(e), + )) => Some(dashcore::OutPoint { + txid: e.transaction_id(), + vout: e.output_index() as u32, + }), + _ => None, + } +} + /// Check whether a platform-wallet error represents a *Core-side* /// InstantSend lock timeout (the asset-lock manager waited the full /// timeout for an IS-lock proof and never observed one). 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 c0e81a90dbc..5d190086ccd 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; @@ -252,7 +252,31 @@ impl AssetLockManager { let proof = match status { AssetLockStatus::Built => { // Re-broadcast and wait for proof. - self.broadcaster.broadcast(&tx).await?; + // + // Only a DEFINITE rejection stops the resume. `MaybeSent` + // means the outcome is unknown — and for a lock stuck at + // `Built` that is the expected answer when the app died + // between a successful broadcast and this status advance: + // the tx is in a mempool (or mined) and every re-broadcast + // reports the same ambiguity. Failing on it left the lock at + // `Built` forever, so each recovery pass repeated the same + // broadcast and the same abort, and the top-up never + // completed. Advancing to `Broadcast` and waiting matches + // what the `Broadcast` arm below already does with the + // identical signal. + match self.broadcaster.broadcast(&tx).await { + Ok(_) => {} + Err(BroadcastError::MaybeSent { reason }) => { + tracing::warn!( + outpoint = %out_point, + reason = %reason, + "resume_asset_lock: re-broadcast of a Built lock returned an \ + unknown outcome (the network may already hold this tx); \ + advancing to Broadcast and waiting for proof" + ); + } + Err(rejected) => return Err(rejected.into()), + } let cs = self .advance_asset_lock_status(out_point, AssetLockStatus::Broadcast, None) .await?; @@ -464,7 +488,9 @@ mod tests { ClientStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, }; use crate::error::PlatformWalletError; - use crate::test_support::{funded_wallet_manager, AlwaysRejectedBroadcaster}; + use crate::test_support::{ + funded_wallet_manager, AlwaysMaybeSentBroadcaster, AlwaysRejectedBroadcaster, + }; use crate::wallet::asset_lock::manager::AssetLockManager; use crate::wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; use crate::wallet::core::WalletGeneration; @@ -647,6 +673,116 @@ mod tests { ); } + /// Builds a tracked `Built`-status lock on a funded wallet and resumes it + /// through `broadcaster`, returning the resume error and the lock's status + /// afterwards. Shared by the two ambiguity/rejection cases below. + async fn resume_built_lock_with( + broadcaster: Arc, + ) -> (PlatformWalletError, AssetLockStatus) { + let (wallet_manager, wallet_id, _balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let sdk = Arc::new( + dash_sdk::SdkBuilder::new_mock() + .with_network(Network::Testnet) + .build() + .expect("mock sdk"), + ); + let manager = AssetLockManager::new( + sdk, + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(Notify::new()), + broadcaster, + WalletPersister::new(wallet_id, Arc::new(RecordingPersistence::default())), + ); + let (transaction, _path) = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityTopUp, + 4, + &signer, + ) + .await + .expect("build asset lock"); + let out_point = OutPoint::new(transaction.txid(), 0); + { + let mut wm = wallet_manager.write().await; + wm.get_wallet_info_mut(&wallet_id) + .expect("wallet must remain registered") + .tracked_asset_locks + .insert( + out_point, + TrackedAssetLock { + out_point, + transaction, + account_index: 0, + funding_type: AssetLockFundingType::IdentityTopUp, + identity_index: 4, + amount: 1_000_000, + status: AssetLockStatus::Built, + proof: None, + }, + ); + } + + let error = manager + .resume_asset_lock(&out_point, Some(Duration::from_millis(10))) + .await + .expect_err("no proof event should arrive in either case"); + let status = wallet_manager + .read() + .await + .get_wallet_info(&wallet_id) + .expect("wallet") + .tracked_asset_locks + .get(&out_point) + .expect("lock stays tracked") + .status + .clone(); + (error, status) + } + + /// An AMBIGUOUS re-broadcast must not end the resume. A lock sitting at + /// `Built` whose transaction was in fact already broadcast (app killed + /// between the send and the status advance) draws `MaybeSent` on every + /// retry, so failing on it pinned the lock at `Built` forever and the + /// top-up could never complete. It must advance to `Broadcast` and go on + /// to wait for the proof — here, until the 10ms test timeout. + #[tokio::test] + async fn built_resume_survives_an_ambiguous_rebroadcast_and_advances() { + let (error, status) = resume_built_lock_with(Arc::new(AlwaysMaybeSentBroadcaster)).await; + + assert!( + matches!(error, PlatformWalletError::FinalityTimeout(_)), + "resume must reach the proof wait, not fail on the broadcast: {error:?}" + ); + assert_eq!( + status, + AssetLockStatus::Broadcast, + "an ambiguous re-broadcast must still advance the lock, or every \ + later pass repeats the same broadcast and the same failure" + ); + } + + /// A DEFINITE rejection is the opposite case and must keep failing the + /// resume: nothing is on the network, so no proof can ever arrive, and + /// the lock stays at `Built` for a later retry to re-send. + #[tokio::test] + async fn built_resume_still_fails_on_a_definite_rejection() { + let (error, status) = resume_built_lock_with(Arc::new(AlwaysRejectedBroadcaster)).await; + + assert!( + matches!(error, PlatformWalletError::TransactionBroadcast(_)), + "a definite rejection must surface as a broadcast failure: {error:?}" + ); + assert_eq!( + status, + AssetLockStatus::Built, + "a tx that never entered the network must stay resumable at Built" + ); + } + /// A lazily-created `IdentityTopUp` funding account must survive a /// restart. Its persisted registration round (account xpub + pool /// snapshot) is the ONLY record the load path can rebuild the account diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs b/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs index e0924008335..5bad34cf7a5 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs @@ -65,7 +65,9 @@ use dash_sdk::platform::transition::put_identity::PutIdentity; use dash_sdk::platform::transition::put_settings::PutSettings; use dash_sdk::platform::transition::top_up_identity::TopUpIdentity; -use crate::error::{is_instant_lock_proof_invalid, PlatformWalletError}; +use crate::error::{ + asset_lock_already_consumed_out_point, is_instant_lock_proof_invalid, PlatformWalletError, +}; use crate::wallet::asset_lock::orchestration::{ out_point_from_proof, submit_with_cl_height_retry, FundingResolution, ResolvedFunding, }; @@ -261,7 +263,15 @@ impl IdentityWallet { .await .map_err(PlatformWalletError::Sdk)? } - Err(e) => return Err(PlatformWalletError::Sdk(e)), + // See the matching arm in `top_up_identity_with_funding`: a + // credit output Platform already spent is terminal, and this is + // the only place that can record it locally on the failure path. + Err(e) => { + if let Some(out_point) = asset_lock_already_consumed_out_point(&e) { + return Err(self.settle_already_consumed_lock(out_point).await); + } + return Err(PlatformWalletError::Sdk(e)); + } }; // Step 4 (best-effort): bookkeeping — add to local @@ -492,7 +502,18 @@ impl IdentityWallet { .await .map_err(PlatformWalletError::Sdk)? } - Err(e) => return Err(PlatformWalletError::Sdk(e)), + // Platform says this credit output was already spent — the + // top-up it would have paid for landed earlier. Record that + // locally (nothing else ever does: the success path is the only + // other caller of `consume_asset_lock`) so the lock leaves the + // resumable set instead of being retried against the same + // deterministic rejection forever. + Err(e) => { + if let Some(out_point) = asset_lock_already_consumed_out_point(&e) { + return Err(self.settle_already_consumed_lock(out_point).await); + } + return Err(PlatformWalletError::Sdk(e)); + } }; // Step 4 (best-effort): persist the new balance + clean up the @@ -550,6 +571,43 @@ impl IdentityWallet { } } +impl IdentityWallet { + /// Record Platform's "already completely used" verdict for `out_point` + /// locally and return the typed error describing it. + /// + /// The credits this lock paid for exist on chain — an earlier attempt + /// succeeded and the client never learned. Marking it + /// [`Consumed`](crate::wallet::asset_lock::tracked::AssetLockStatus::Consumed) + /// takes it out of the resumable set, which is what stops a recovery + /// worker retrying it on every pass (and, for clients that block new + /// funding while a lock is unresolved, unblocks the next purchase). + /// + /// Returns [`AssetLockAlreadyConsumed`](PlatformWalletError::AssetLockAlreadyConsumed) + /// — the same typed error a resume of an already-consumed lock raises, so + /// callers need one terminal case, not a Platform error-string match. + /// A bookkeeping failure here can only be `WalletNotFound`; it is logged + /// rather than returned, because the verdict itself is what the caller + /// must act on. + async fn settle_already_consumed_lock( + &self, + out_point: dashcore::OutPoint, + ) -> PlatformWalletError { + tracing::info!( + outpoint = %out_point, + "Platform rejected the asset lock as already completely used — its \ + credits landed on an earlier attempt; marking the lock consumed" + ); + if let Err(e) = self.asset_locks.consume_asset_lock(&out_point).await { + tracing::warn!( + outpoint = %out_point, + error = %e, + "consume_asset_lock failed after Platform's already-used rejection" + ); + } + PlatformWalletError::AssetLockAlreadyConsumed(out_point) + } +} + // --------------------------------------------------------------------------- // Helpers // ---------------------------------------------------------------------------