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
|
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 (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes update asset-lock dispatch and recovery handling for concurrent and transport-dependent outcomes. They also add typed persistence errors across Rust, FFI, Kotlin, and Swift, with retry classification, user-facing messages, and preserved diagnostics. ChangesAsset-lock dispatch and recovery
Typed persistence error contracts
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ResumeAssetLock
participant TransactionBroadcaster
participant WalletManager
ResumeAssetLock->>TransactionBroadcaster: defensive re-broadcast
TransactionBroadcaster-->>ResumeAssetLock: readiness or rejection result
ResumeAssetLock->>WalletManager: refresh conflict and local finality
WalletManager-->>ResumeAssetLock: contested verdict, proof, or lookup error
Merge Risk: ⚪ Minimal · up to The asset-lock recovery and typed persistence error changes have no remaining concrete 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 751cd3b) · triage: normal · Phase 2 only (queue backlog) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4636 +/- ##
============================================
- Coverage 86.36% 86.00% -0.37%
============================================
Files 2766 2766
Lines 366105 367711 +1606
============================================
+ Hits 316191 316250 +59
- Misses 49914 51461 +1547
🚀 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 ( |
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.
… 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.
Return the contested verdict immediately when transport readiness was missed and the defensive re-broadcast was rejected before dispatch. The readiness-deferred retry owns the next proof wait, while ready transports retain the bounded wait. This intermediate fast path returns the conflict snapshot taken before readiness. That bounds offline latency but introduces a stale-verdict hazard if finality lands during the post-rejection probe; the follow-up commit refreshes finality and the conflict before code 48. Test would have caught this in CI: ✖ on a160f11: offline_broadcast_resume_with_a_conflict_skips_the_dead_proof_wait failed "the offline foreground resume must not add the default proof wait" with left: 195s, right: 15s ✔ here: the same test returns AssetLockInputContested after exactly the 15s readiness wait
Refresh local finality and the input conflict immediately before returning code 48 from the offline Broadcast fast path. A recoverable proof completes the resume; finalized evidence without a proof suppresses the contested verdict, while only a genuine FinalityTimeout becomes code 20 and lookup errors such as WalletNotFound propagate unchanged.
Document the refreshed snapshot as the verdict linearization point: code 48 remains provisional, and a proof arriving afterwards is reported by the next resume. Settlement now explicitly requires a recoverable proof.
Tests would have caught this in CI:
✖ on commit A: offline_broadcast_resume_refreshes_finality_before_reporting_a_conflict failed "fresh local finality must outrank the stale conflict snapshot: AssetLockInputContested { ... }"
✔ here: the same test returns the ChainLock proof after exactly the 15s readiness wait
✖ on commit A: offline_broadcast_resume_preserves_wallet_removal_during_refresh failed "a removed wallet must report WalletNotFound, got AssetLockInputContested { ... }"
✔ here: the same test returns WalletNotFound after exactly the 15s readiness wait
7907237 to
751cd3b
Compare
|
Thanks — all three landed, plus one defect your second point led me into. The 180s offline dead wait is gone. When transport readiness is missed and the That fast exit introduced a worse bug than it fixed, so it is now two commits. Returning A removed wallet was being reported as still tracked. Chasing the above, the refresh Rebased, so the stray rustfmt-only Two things I deliberately did not do, both recorded rather than silently skipped:
Worth flagging for mobile hosts: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The reviewed changes correctly address both in-scope defects: the create path now advances only rows that remain in Built, preventing a concurrent finalized status from being downgraded, and the Broadcast recovery path preserves contested evidence while refreshing finality before returning a verdict. The targeted tests and documentation updates cover the changed behavior, and no additional in-scope correctness issues were identified.
Review provenance
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)
- Triage:
normalbygpt-6-astra(effort low) — The diff makes substantial, intricate changes across asset-lock creation and recovery state-machine logic, but it does not itself alter consensus rules, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 15 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Review pre-existing WalletNotFound error mapping across mobile bindings — The Rust wallet can propagate WalletNotFound, while the existing FFI and mobile catch-up wrappers normalize it to generic error codes. This mapping asymmetry is outside the production changes in this PR, which modify recovery behavior and documentation rather than the executable error mapping.
- Follow-up: Track the WalletNotFound mapping contract separately and add cross-language tests if mobile callers need to distinguish a removed wallet from generic errors.
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
Summary by CodeRabbit
New Features
Bug Fixes
Documentation