fix(platform-wallet): settle already-consumed asset locks and survive an ambiguous resume broadcast - #4337
Conversation
…biguous resume broadcast 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 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe wallet now detects already-consumed asset-lock outputs during registration and top-up. It locally settles affected locks and returns a typed error. Asset-lock recovery now treats ambiguous rebroadcast results as broadcasted and waits for proof. ChangesAsset-lock handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit 3fd9644) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- Around line 266-274: Classify already-consumed errors in both ChainLock retry
failure paths before propagating them: update the registration retry handling at
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:266-274
and the top-up retry handling at
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:505-516
to reuse asset_lock_already_consumed_out_point and settle_already_consumed_lock,
while preserving PlatformWalletError::Sdk for other errors.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e067e1cd-9996-4e1d-b40e-cad8d75452b0
📒 Files selected for processing (3)
packages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/identity/network/registration.rs
| // 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)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle already-consumed errors from both ChainLock retries.
The IS-lock fallback branches propagate a failed ChainLock retry before the new generic error arms execute. Therefore, an already-consumed rejection after an IS-to-CL upgrade remains PlatformWalletError::Sdk and the tracked lock remains resumable.
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs#L266-L274: classify the ChainLock registration retry error before propagating it.packages/rs-platform-wallet/src/wallet/identity/network/registration.rs#L505-L516: classify the ChainLock top-up retry error before propagating it.
📍 Affects 1 file
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs#L266-L274(this comment)packages/rs-platform-wallet/src/wallet/identity/network/registration.rs#L505-L516
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`
around lines 266 - 274, Classify already-consumed errors in both ChainLock retry
failure paths before propagating them: update the registration retry handling at
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:266-274
and the top-up retry handling at
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:505-516
to reuse asset_lock_already_consumed_out_point and settle_already_consumed_lock,
while preserving PlatformWalletError::Sdk for other errors.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The typed consumed-error extraction and ambiguous-broadcast recovery are directionally correct, but the exact head still has three blocking defects: uncertain pre-send failures can enter an indefinite finality wait, ChainLock fallback submissions bypass consumed-lock settlement, and unauthenticated DAPI errors can permanently tombstone valid locks. The new consumed classification and settlement path also lacks regression coverage.
Source: reviewer backends codex general/security-auditor/rust-quality (gpt-5.6-sol); final verifier backend codex (gpt-5.6-sol). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:269-284: Preserve a retry path when an ambiguous broadcast never sent the transaction
`DapiBroadcaster` maps every DAPI failure to `MaybeSent`, including outages where no request delivered the transaction to Core. This branch nevertheless persists `Broadcast` and waits for proof. In the identity funding resolver, the bounded proof timeout becomes `FundingResolution::IsTimeout`, after which registration and top-up call `upgrade_to_chain_lock_proof(..., None)` and wait indefinitely. If the transaction was never sent, no InstantSend or ChainLock proof can ever arrive, and the operation no longer gets another opportunity to rebroadcast. Keep the uncertain acceptance state retryable: after the bounded proof wait expires without positive network evidence, surface `TransactionBroadcastUnconfirmed` or otherwise restore a state that permits another broadcast instead of entering the unbounded ChainLock fallback.
In `packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:253-264: Route ChainLock fallback rejections through consumed-lock settlement
The outer submission error is classified with `asset_lock_already_consumed_out_point`, but the nested ChainLock retry short-circuits through `.map_err(PlatformWalletError::Sdk)?`. The same bypass exists in the top-up path at lines 493-503. If an earlier ambiguous submission commits, or another recovery consumes the outpoint while this flow waits for a ChainLock, the fallback can return `IdentityAssetLockTransactionOutPointAlreadyConsumedError`; the current code leaves the lock resumable and returns a generic SDK error, recreating the terminal retry loop this PR is intended to fix. Normalize both the initial and fallback submission results through the same consumed-error settlement path.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:591-607: Do not permanently settle a lock from an unauthenticated DAPI rejection
`settle_already_consumed_lock` irreversibly persists `Consumed` solely from a DAPI-provided consensus error. That verdict is not authenticated: the SDK returns wait-stream errors before GroveDB proof and Tenderdash quorum-signature verification (`rs-sdk/src/platform/transition/broadcast.rs:347-397`), while `Protocol(ConsensusError)` can be deserialized directly from unauthenticated gRPC metadata (`rs-sdk/src/error.rs:176-200`). Both error shapes are non-retryable, so one malicious DAPI node can fabricate an already-consumed error, name the submitted outpoint, and remove an actually unspent lock from the wallet's resumable funding set. Binding the reported outpoint to the submitted proof prevents unrelated-lock corruption but does not authenticate the verdict; permanent settlement must wait for quorum-authenticated state evidence, or the lock must remain unsettled and retryable.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:534-556: Add regression coverage for consumed-error classification and settlement
The two new tests cover only the `Built` rebroadcast behavior. There is no test proving that `asset_lock_already_consumed_out_point` extracts the correct outpoint from both supported SDK wrappers, rejects unrelated errors, or that the resulting settlement persists `Consumed` and removes the lock from the resumable set. The adjacent address-nonce classifier already demonstrates the expected wrapper-by-wrapper test pattern. Add equivalent classifier tests and an end-to-end wallet-state assertion for the consumed settlement path.
| .await | ||
| .map_err(PlatformWalletError::Sdk)? |
There was a problem hiding this comment.
🔴 Blocking: Route ChainLock fallback rejections through consumed-lock settlement
The outer submission error is classified with asset_lock_already_consumed_out_point, but the nested ChainLock retry short-circuits through .map_err(PlatformWalletError::Sdk)?. The same bypass exists in the top-up path at lines 493-503. If an earlier ambiguous submission commits, or another recovery consumes the outpoint while this flow waits for a ChainLock, the fallback can return IdentityAssetLockTransactionOutPointAlreadyConsumedError; the current code leaves the lock resumable and returns a generic SDK error, recreating the terminal retry loop this PR is intended to fix. Normalize both the initial and fallback submission results through the same consumed-error settlement path.
source: ['codex', 'coderabbit']
| 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) |
There was a problem hiding this comment.
🔴 Blocking: Do not permanently settle a lock from an unauthenticated DAPI rejection
settle_already_consumed_lock irreversibly persists Consumed solely from a DAPI-provided consensus error. That verdict is not authenticated: the SDK returns wait-stream errors before GroveDB proof and Tenderdash quorum-signature verification (rs-sdk/src/platform/transition/broadcast.rs:347-397), while Protocol(ConsensusError) can be deserialized directly from unauthenticated gRPC metadata (rs-sdk/src/error.rs:176-200). Both error shapes are non-retryable, so one malicious DAPI node can fabricate an already-consumed error, name the submitted outpoint, and remove an actually unspent lock from the wallet's resumable funding set. Binding the reported outpoint to the submitted proof prevents unrelated-lock corruption but does not authenticate the verdict; permanent settlement must wait for quorum-authenticated state evidence, or the lock must remain unsettled and retryable.
source: ['codex']
| pub fn asset_lock_already_consumed_out_point( | ||
| error: &dash_sdk::Error, | ||
| ) -> Option<dashcore::OutPoint> { | ||
| 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, | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for consumed-error classification and settlement
The two new tests cover only the Built rebroadcast behavior. There is no test proving that asset_lock_already_consumed_out_point extracts the correct outpoint from both supported SDK wrappers, rejects unrelated errors, or that the resulting settlement persists Consumed and removes the lock from the resumable set. The adjacent address-nonce classifier already demonstrates the expected wrapper-by-wrapper test pattern. Add equivalent classifier tests and an end-to-end wallet-state assertion for the consumed settlement path.
source: ['codex']
2 participants
Issue being fixed or feature implemented
Two ways a completed or in-flight asset-lock top-up could never finish, both observed live on an Android testnet wallet (MO-998 / dashpay/dash-wallet#1520):
1. Platform's "already completely used" verdict was dropped. When a transition is rejected with
IdentityAssetLockTransactionOutPointAlreadyConsumedError("Asset lock transaction {txid} output {n} already completely used"), the credits it would have bought already landed — an earlier attempt succeeded and the client never learned. Nothing recorded that locally:consume_asset_lockwas 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, forever. A client that blocks new funding while a lock is unresolved could not buy credits at all until it special-cased the error string (which the Android wallet currently does — this PR is the typed root fix that lets that workaround be deleted).2. A
Built-status resume aborted on an ambiguous re-broadcast. TheBuiltarm ofresume_asset_lockpropagated every broadcast error, includingMaybeSent. For a lock stuck atBuiltwhose transaction WAS broadcast (the app died between the send and the status advance),MaybeSentis the expected answer on every retry — DAPI classifies all failures that way — so the resume failed, the lock stayedBuilt, and the next pass repeated it. The top-up never completed.What was done?
register_identity_with_funding,top_up_identity_with_funding) classify the already-consumed rejection from the typed consensus error (asset_lock_already_consumed_out_point, reading the outpoint Platform itself names — never a message match), mark the lockConsumedvia the existingconsume_asset_lock, and return the same typedAssetLockAlreadyConsumeda resume of a consumed lock already raises — callers need one terminal case.Builtresume arm now distinguishes verdicts: only a definiteRejectedstops the resume (nothing is on the network; the lock stays atBuiltfor a later re-send).MaybeSentadvances toBroadcastand proceeds to the proof wait — matching what theBroadcastarm already does with the identical signal.How Has This Been Tested?
Built → Broadcastand reach the proof wait; a definite rejection must keep failing with the lock still resumable atBuilt.v4.2-devtip (rebased over feat(platform-wallet): CoinJoin-drain asset-lock funding for the shielded pool #4327):cargo test -p platform-wallet --lib— 579 passed, 0 failed.cargo clippy -p platform-wallet --lib --tests— clean.resume_asset_lockretries against the same rejection; a stuckBuiltlock re-broadcast on every recovery pass).Breaking Changes
None. Error surfaces gain no new variants; a rejection that previously surfaced as a generic SDK error now surfaces as the existing typed
AssetLockAlreadyConsumed.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit