fix(platform-wallet): close asset-lock resume broadcast race#4016
fix(platform-wallet): close asset-lock resume broadcast race#4016thepastaclaw wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…t_lock `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) <noreply@anthropic.com>
39348ea to
0efd12c
Compare
|
⛔ Blockers found — Sonnet deferred (commit 0efd12c) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The pre-broadcast status promotion fixes the intended single-resume interleave, but the newly added rejection rollback is not owned by the resume attempt that performed the promotion. Concurrent resume calls can therefore downgrade another successful broadcast to Built, allowing the create-side rejection cleanup to delete the row and release inputs for a transaction that may already be propagating.
Validated blockers were found in the Codex precheck. Sonnet 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)
🔴 1 blocking
🤖 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:275-305: Rollback can clobber a concurrent successful resume
The write lock protects only the individual status mutation, not the promote/broadcast/rollback sequence across the broadcast await. Two callers can resume the same `Built` lock: caller A promotes it to `Broadcast`, while caller B snapshots either `Built` or `Broadcast` and successfully broadcasts the transaction. If A then receives `Rejected`, this filter still sees `Broadcast` because B remains at that status while waiting for proof, so A downgrades the shared row to `Built`. If the original create-side broadcast subsequently returns `Rejected`, `build.rs:530-550` sees `Built`, removes the row, and releases the funding reservation even though B handed the transaction to the network. Concurrent calls are supported by the public Rust and FFI resume entry points, and neither the manager nor the catch-up path serializes operations per outpoint. Tie rollback to exclusive attempt ownership with a per-outpoint single-flight guard or generation token, or conservatively retain `Broadcast` on rejection; the existing `Broadcast` arm already re-broadcasts on future resumes.
Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
| Err(BroadcastError::Rejected { reason }) => { | ||
| // The tx provably didn't reach the network, but we | ||
| // already pushed the row to `Broadcast` as the | ||
| // race-guard against a concurrent create-path | ||
| // cleanup. Roll the status back to `Built` so a | ||
| // later resume actually re-broadcasts — otherwise | ||
| // a never-sent tx sits at `Broadcast` forever and | ||
| // both this call's `wait_for_proof` and every | ||
| // subsequent resume's `Broadcast`-arm defensive | ||
| // rebroadcast+wait wait for a proof that can't | ||
| // come. Guarded on the current status: a | ||
| // concurrent path (SPV proof arrival, another | ||
| // resume that already broadcast successfully) may | ||
| // have advanced past `Broadcast` with real state | ||
| // we must not clobber. | ||
| let rollback_cs = { | ||
| let mut wm = self.wallet_manager.write().await; | ||
| wm.get_wallet_info_mut(&self.wallet_id) | ||
| .and_then(|info| info.tracked_asset_locks.get_mut(out_point)) | ||
| .filter(|entry| entry.status == AssetLockStatus::Broadcast) | ||
| .map(|entry| { | ||
| entry.status = AssetLockStatus::Built; | ||
| let mut cs = AssetLockChangeSet::default(); | ||
| cs.asset_locks.insert(*out_point, (&*entry).into()); | ||
| cs | ||
| }) | ||
| }; | ||
| if let Some(cs) = rollback_cs { | ||
| self.queue_asset_lock_changeset(cs); | ||
| } | ||
| return Err(BroadcastError::Rejected { reason }.into()); |
There was a problem hiding this comment.
🔴 Blocking: Rollback can clobber a concurrent successful resume
The write lock protects only the individual status mutation, not the promote/broadcast/rollback sequence across the broadcast await. Two callers can resume the same Built lock: caller A promotes it to Broadcast, while caller B snapshots either Built or Broadcast and successfully broadcasts the transaction. If A then receives Rejected, this filter still sees Broadcast because B remains at that status while waiting for proof, so A downgrades the shared row to Built. If the original create-side broadcast subsequently returns Rejected, build.rs:530-550 sees Built, removes the row, and releases the funding reservation even though B handed the transaction to the network. Concurrent calls are supported by the public Rust and FFI resume entry points, and neither the manager nor the catch-up path serializes operations per outpoint. Tie rollback to exclusive attempt ownership with a per-outpoint single-flight guard or generation token, or conservatively retain Broadcast on rejection; the existing Broadcast arm already re-broadcasts on future resumes.
| Err(BroadcastError::Rejected { reason }) => { | |
| // The tx provably didn't reach the network, but we | |
| // already pushed the row to `Broadcast` as the | |
| // race-guard against a concurrent create-path | |
| // cleanup. Roll the status back to `Built` so a | |
| // later resume actually re-broadcasts — otherwise | |
| // a never-sent tx sits at `Broadcast` forever and | |
| // both this call's `wait_for_proof` and every | |
| // subsequent resume's `Broadcast`-arm defensive | |
| // rebroadcast+wait wait for a proof that can't | |
| // come. Guarded on the current status: a | |
| // concurrent path (SPV proof arrival, another | |
| // resume that already broadcast successfully) may | |
| // have advanced past `Broadcast` with real state | |
| // we must not clobber. | |
| let rollback_cs = { | |
| let mut wm = self.wallet_manager.write().await; | |
| wm.get_wallet_info_mut(&self.wallet_id) | |
| .and_then(|info| info.tracked_asset_locks.get_mut(out_point)) | |
| .filter(|entry| entry.status == AssetLockStatus::Broadcast) | |
| .map(|entry| { | |
| entry.status = AssetLockStatus::Built; | |
| let mut cs = AssetLockChangeSet::default(); | |
| cs.asset_locks.insert(*out_point, (&*entry).into()); | |
| cs | |
| }) | |
| }; | |
| if let Some(cs) = rollback_cs { | |
| self.queue_asset_lock_changeset(cs); | |
| } | |
| return Err(BroadcastError::Rejected { reason }.into()); | |
| Err(e @ BroadcastError::Rejected { .. }) => { | |
| return Err(e.into()); | |
| } |
source: ['codex']
1 participant
Follow-up to merged #3985 / tracker thepastaclaw/tracker#1742.
The later automated review on #3985 found a real read-before-broadcast race:
resume_asset_lockcould snapshot a Built row, then a concurrent create-path Rejected cleanup could delete the row and release the reservation before resume rebroadcasted the same tx.This PR:
Validation:
cargo test -p platform-wallet --lib asset_lock::cargo clippy -p platform-wallet --lib --testscargo build -p platform-wallet --testsgit show --check HEADNote: the race regression test was also verified to fail against the pre-fix implementation.