fix(platform-wallet): close the asset-lock resume broadcast race - #4636
fix(platform-wallet): close the asset-lock resume broadcast race#4636shumkov wants to merge 4 commits into
Conversation
Promote Built rows before resume broadcasts through a shared compare-and-set. Preserve concurrently advanced status and proof in both resume and create paths, and keep rejected attempts tracked at Broadcast without releasing their inputs. Test would have caught this in CI: - rejected_create_while_resume_broadcasts_keeps_row_and_reservation: ✖ before the fix the rejected create removed the row and released its reservation; ✔ after the fix the row remains Broadcast and a rebuild cannot select its inputs. - stale_built_resume_does_not_downgrade_a_concurrently_finalized_row: ✖ before the fix the stale resume timed out after replacing ChainLocked with Broadcast; ✔ after the fix it re-dispatches from the attached ChainLock proof. - create_broadcast_does_not_downgrade_a_concurrently_finalized_row: ✖ before the fix the create completion replaced ChainLocked with Broadcast; ✔ after the fix it preserves the finalized status and proof. - Built-resume rejection assertions: ✖ before the fix the row stayed Built; ✔ after the fix it stays tracked at Broadcast for defensive resume.
Keep definite rejections for Broadcast rows on the bounded proof-wait path when a standing input conflict exists, so later resumes reproduce AssetLockInputContested. Update status wording to reflect pre-send promotion. Test would have caught this in CI: - a_rejected_rebroadcast_of_a_conflicted_built_lock_reports_the_contested_verdict: ✖ before the fix the second resume returned TransactionBroadcastUnconfirmed; ✔ after the fix it returns AssetLockInputContested while the row remains Broadcast.
… send A Broadcast row now means a broadcast was attempted, not that one reached the network: two pre-dispatch rejections can leave a row at Broadcast having sent nothing. The contested-verdict docs in the Rust error type and both mobile SDKs still asserted an earlier call had sent the transaction. Docs only; no behaviour change, so no test accompanies it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAsset-lock creation and recovery now use conditional status advancement and RAII dispatch claims. Concurrent finalization, rejection, cancellation, and contested-proof scenarios preserve tracked state and verdicts. Related SDK documentation and one test formatting change were updated. ChangesAsset-lock race handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CreateOrResume
participant TransactionBroadcaster
participant AssetLockManager
participant ProofWait
CreateOrResume->>TransactionBroadcaster: send or re-send asset-lock transaction
TransactionBroadcaster-->>CreateOrResume: accepted or rejected
CreateOrResume->>AssetLockManager: conditionally update tracked status
AssetLockManager-->>CreateOrResume: preserve concurrent status and proof
CreateOrResume->>ProofWait: wait when an input conflict is sighted
ProofWait-->>CreateOrResume: contested or final proof verdict
Merge Risk: ⚪ Minimal · up to Asset-lock create and resume flows now retain state during concurrent dispatch and finalization, preventing stale cleanup or status downgrades. The covered race and recovery behavior has no identified current-head merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Final review complete — no blockers (commit 7907237) · triage: critical · Phase 2 only (queue backlog) |
Keep #4355's resume dispatch-claim and transport-readiness flow, and conditionally advance the create path after broadcast. Test would have caught this in CI: ✖ the unconditional merged-parent advance downgraded ChainLocked to Broadcast; ✔ the conditional advance preserves the finalized row and proof.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4636 +/- ##
============================================
- Coverage 85.34% 84.51% -0.84%
============================================
Files 2795 2796 +1
Lines 373566 375112 +1546
============================================
- Hits 318827 317025 -1802
- Misses 54739 58087 +3348
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the changes at head 7907237; no actionable in-scope defects were found. The conditional create-path status update preserves concurrent finality, and rejected defensive re-broadcasts retain bounded conflict resolution without releasing reservations. Independent validation passed all 98 targeted asset-lock tests, the full platform-wallet and platform-wallet-ffi suites (1,361 passed, 5 ignored), and git diff --check; the worktree remains clean.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — The change modifies concurrent asset-lock state transitions, broadcast recovery, and funding-reservation protection, where incorrect ordering or status handling could release committed inputs, create conflicting transactions, or compromise wallet fund recovery. - Phase 1 reviewers: not run (skipped for throughput: 21 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
|
Went through the diff against the merge-base (299d662). The code changes look right to me, a few things before I approve: 1. The description no longer matches the diff. It describes a 2. 180 s wait on an offline launch ( 3. Rebase please. The merge commit picked up a rustfmt-only hunk in Minor, no action needed unless you are already in there: the code-48 docs ( |
3 participants
Issue being fixed or feature implemented
Two defects on the asset-lock resume and create paths, both reachable today on
v4.2-dev.The create path could downgrade a concurrently finalized row. After its broadcast,
broadcast_funded_asset_lockadvanced the row toBroadcastunconditionally. If another flow carried the same lock toInstantSendLockedorChainLockedwhile that broadcast was in flight, the advance overwrote the stronger status and persistedBroadcastwith no proof.A contested
Broadcastrow reported the wrong verdict. When a re-broadcast was rejected and an input conflict had been sighted, the Broadcast arm returnedTransactionBroadcastUnconfirmedimmediately, so the caller never sawAssetLockInputContested— the Built arm already had the opposite behaviour.What was done?
build.rs— the create path's post-broadcast advance now goes throughadvance_asset_lock_status_if(|s| s == Built, Broadcast, None). A row a concurrent flow already carried pastBuiltis left alone, and the call still returnsOk, since the follow-up proof wait resolves from the stronger status.sync/recovery.rs, Broadcast arm — on a rejected re-broadcast with no local proof, aninput_conflictsighting now falls through to the existing bounded wait andinput_conflict_verdictrather than returning early, mirroring the Built arm. The no-sighting path is unchanged.Broadcastas proof the transaction reached the network now say a broadcast was attempted, acrossrs-platform-wallet,rs-platform-wallet-ffiand both mobile SDKs.sync/recovery.rs's Built arm,await_broadcast_ready,resume_when_transport_ready, theResumeDispatchClaimlifecycle and absorbingConsumedare byte-identical to the merge-base.Tests
Test would have caught this in CI: ✖ before the fix, ✔ after — each proven by reverting one production hunk in isolation.
create_broadcast_does_not_downgrade_a_concurrently_finalized_row— revert only thebuild.rshunk and it fails withleft: Broadcast, right: ChainLocked.a_rejected_rebroadcast_of_a_conflicted_built_lock_reports_the_contested_verdict— extended to a second resume with the row already atBroadcast. Revert only therecovery.rshunk and it fails in that second-resume match withTransactionBroadcastUnconfirmed; the failure is past the whole first half, so the base's Built-arm assertions still pass and it is precisely the Broadcast-arm extension the base cannot satisfy.Two tests from the original CAS design were removed because they can no longer fail, not because they now fail:
rejected_create_while_resume_broadcasts_keeps_row_and_reservationuniquely pinned the abandoned promote-before-broadcast ordering, and every safety assertion it carried is asserted by the base'sa_rejection_cleanup_cannot_release_inputs_under_a_parked_resume;stale_built_resume_does_not_downgrade_a_concurrently_finalized_rowdepended on a test-only hook that only existed alongside the CAS.One behaviour worth stating plainly: under #4355's design a Built resume that snapshotted before a concurrent finalization still makes a benign redundant re-send. The network answers already-known,
advance_if(Built)leaves the row alone, and the wait resolves from the row's proof. That is accepted base behaviour, not introduced here, and it is why the deleted test's "must not broadcast" half is gone.cargo test -p platform-walletand-p platform-wallet-ffipass; clippy-D warningsandfmt --checkclean.Supersedes #4016, which is closed.
🤖 Generated with Claude Code