fix(platform-wallet): re-broadcast a Broadcast-status lock on resume#4009
Conversation
Resuming a tracked asset lock at `Broadcast` status only waited for a proof; it never re-broadcast the funding tx (only the `Built` arm does). If the tx had been evicted from every mempool (Core's default `-mempoolexpiry` is two weeks) or the original broadcast reached no peers (an SPV connectivity gap), no InstantSend/ChainLock proof could ever arrive — and since the user-facing funding flows now wait for the ChainLock indefinitely (#4006), the resume would hang forever with no recovery path. Add a defensive re-broadcast to the `Broadcast` arm of `resume_asset_lock` before the wait. Unlike the `Built` arm (whose tx was never broadcast, so a broadcast error is genuine and propagates), a `Broadcast`-status tx may still be in a mempool or already mined, where the network reports "already known" / "already in block chain". The broadcaster can't distinguish that from a real rejection (DAPI classifies every failure as `MaybeSent`), so the re-broadcast is best-effort: log the error and proceed to `wait_for_proof` regardless rather than failing a resume on a tx that is actually fine (if it was mined, the wait resolves immediately from the persisted record). This path is shared by the identity, platform-address, and shielded resume flows plus the app-launch catch-up sweep; re-broadcasting an already-known tx is idempotent, so the extra call is harmless where the tx is still live. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
|
✅ Review complete (commit 35a8885) |
There was a problem hiding this comment.
Code Review
Small, well-scoped defensive fix that adds a best-effort re-broadcast to resume_asset_lock's Broadcast arm before waiting for a proof. Implementation is correct — the swallowed broadcaster error is documented and matches DAPI's MaybeSent semantics. All four reviewers converged on the same gap: the new arm has no unit test, even though the crate's generic TransactionBroadcaster and existing test doubles (AlwaysMaybeSentBroadcaster, RejectFirstBroadcaster) make this cheap to pin down.
Source: reviewers=claude-general(opus), codex-general(gpt-5.5), claude-security-auditor(opus), codex-security-auditor(gpt-5.5), claude-rust-quality(opus), codex-rust-quality(gpt-5.5); verifier=claude(opus)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 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`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:267-301: New `Broadcast`-arm re-broadcast has no unit test
The functional fix in this PR is the side effect added to `resume_asset_lock`'s `AssetLockStatus::Broadcast` arm: it must (a) invoke `self.broadcaster.broadcast(&tx).await` before entering `wait_for_proof`, and (b) continue to `wait_for_proof` even if that re-broadcast returns `Err`. Neither invariant is exercised by the existing asset-lock tests, which cover the initial-broadcast rejection/ambiguity paths but not resume from an already-`Broadcast` row.
The crate is set up to test this cheaply — `AssetLockManager` is generic over `B: TransactionBroadcaster + ?Sized`, and the existing test doubles (`AlwaysMaybeSentBroadcaster` in `test_support.rs`, plus the `RejectFirstBroadcaster`-style pattern used in `core/broadcast.rs`) can assert both invariants with a counter and an `Err`-returning broadcaster. Without such a test, a future refactor could silently demote the `if let Err` back to `?` (reintroducing the hang) or drop the re-broadcast call entirely, and CI would still pass. Given the failure mode this PR is defending against is 'resume hangs forever with no in-app recovery', this contract is worth pinning down.
| 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" | ||
| ); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: tracing::debug! may hide genuine broadcaster failures on resume
Swallowing the broadcaster error is correct (DAPI can't distinguish 'already known' / 'already in block chain' from a real rejection because everything uncertain is MaybeSent). But at debug! level, the pathological case this PR is trying to defend against — the broadcaster reaching zero peers — leaves no trace at default log levels, after which the unbounded wait_for_proof from #4006 blocks silently. Promoting to warn! (or info! with a clear 'best-effort re-broadcast reported error' phrasing) would keep the healthy case quiet (the line only fires on Err) while making the failure diagnosable from a normal log capture. Volume isn't a concern since this only runs on resume, not steady-state.
source: ['claude']
2 participants
Follow-up to the ADDR-09 finality work (#4006 / #4007). Surfaced by the review of #4007: resuming an orphaned
Broadcast-status lock could hang forever.Issue being fixed or feature implemented
resume_asset_lock'sBroadcastarm only waited for a proof — it never re-broadcast the funding tx (only theBuiltarm does). If the tx had been evicted from every mempool (Core's default-mempoolexpiryis two weeks) or the original broadcast reached no peers (an SPV connectivity gap — a real condition on the testnet QA setup), no InstantSend/ChainLock proof can ever arrive. And since the user-facing funding flows now wait for the ChainLock indefinitely (#4006), the resume hangs forever: the controller stays.inFlight, Cancel is disabled, the network toggle is disabled app-wide, and no in-app recovery exists short of relaunch. #4007's new "Resume on a Broadcast orphan" button makes this reachable from the UI.What was done?
Add a defensive re-broadcast to the
Broadcastarm ofresume_asset_lockbefore the wait.Unlike the
Builtarm — whose tx was never broadcast, so a broadcast error is genuine and propagates — aBroadcast-status tx may still be in a mempool or already mined, where the network reports "already known" / "already in block chain". The broadcaster can't distinguish that from a real rejection (DapiBroadcasterclassifies every failure asMaybeSent), so the re-broadcast is best-effort: log the error and proceed towait_for_proofregardless, rather than failing a resume on a tx that is actually fine. If it was mined, the wait resolves immediately from the persisted record.This path is shared by the identity, platform-address, and shielded resume flows plus the app-launch catch-up sweep; re-broadcasting an already-known tx is idempotent, so the extra call is harmless where the tx is still live.
How Has This Been Tested?
cargo check -p platform-wallet --features shielded --all-targets— clean.cargo test -p platform-wallet --features shielded asset_lock— 19 asset-lock unit tests pass.cargo fmt --allapplied.Not yet exercised end-to-end (would require an evicted/undelivered asset-lock tx on testnet).
Breaking Changes
None. No consensus or ABI change.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code