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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe wallet detects already-consumed asset-lock outputs, settles matching tracked locks, and returns typed errors. Registration, top-up, and ChainLock retry paths use this handling. Ambiguous rebroadcasts advance to ChangesAsset-lock handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
|
🔍 Review in progress — actively reviewing now (commit 319c4c9) |
![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
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']
…outpoint and classify every submit arm Review follow-ups to the already-consumed settlement: - The settlement moves onto AssetLockManager as settle_reported_consumed and now settles ONLY when Platform's verdict names the outpoint this submission actually carried. The verdict is an unauthenticated error relayed by one DAPI node; binding it stops a fabricated answer from tombstoning an unrelated lock (which wiped its proof and stranded the burned funds). - The IS->CL fallback resubmits in registration, top-up, and the platform-address funding flow previously mapped rejections straight to a generic Sdk error, so a consumed verdict on the second attempt left the lock resumable and the endless-retry loop reachable. All six rejection arms now route through the shared classification. - Tests: classifier extraction from both wire shapes plus unrelated/causeless passthrough (error.rs), and a manager-level test proving an unbound or unrelated rejection leaves the lock untouched while the bound verdict tombstones it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Closing after splitting this in two, per review of the overlap with #4357 (merged 2026-08-11):
Thanks @HashEngineering — both fixes were real; one of them just got raced by a broader implementation. |
3 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