fix(platform-wallet): wait for SPV transport before resuming asset locks that need broadcast - #4355
fix(platform-wallet): wait for SPV transport before resuming asset locks that need broadcast#4355QuantumExplorer wants to merge 3 commits into
Conversation
…cks that need broadcast The app-launch asset-lock catch-up races SPV client startup and loses permanently. Hosts drive the catch-up from wallet load, which runs before `startSpv` — in dashwallet-ios roughly fifty lines and two setup steps before it. `resume_asset_lock`'s `Built` arm broadcasts into a client that has not started, and that failure is classified `BroadcastError::Rejected`, the "provably never sent" verdict. Nothing reschedules the catch-up, so the lock never leaves `Built` and every later session repeats the identical race. One observed lock held 2 DASH for days until a manual retry. Gate the broadcast on transport readiness rather than reordering the hosts. `TransactionBroadcaster` gains `wait_until_ready`, defaulted to "always ready" so `DapiBroadcaster` and the test doubles are unchanged; `SpvBroadcaster` delegates to a new `SpvRuntime::wait_until_ready` that waits for a started client with at least one connected peer — both halves matter, since zero-peers is the other pre-send `Rejected` shape at launch. Only the two arms that actually broadcast wait, and the wait is deducted from the caller's timeout so the total stays inside the requested budget. The `InstantSendLocked` / `ChainLocked` / `RecoveredFromChain` arms already hold a proof and broadcast nothing, so they never wait — that exclusion is load-bearing, not an optimization: four callers pass `timeout: None` on exactly that branch and gating them would turn a cheap path re-derivation into an indefinite hang. A readiness timeout is not fatal; the broadcast is attempted anyway and reports the same error it would have reported without the wait. Regression tests cover all three behaviours: a `Built` resume under the catch-up's unbounded timeout records no broadcast attempt until the transport comes up and then sends the original transaction; a bounded caller still fails fast instead of parking; and a chain-locked lock resumes without consulting readiness at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (9)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAsset-lock recovery now checks local finality before transport access, waits for bounded SPV readiness before eligible broadcasts, and protects dispatch claims from premature cleanup. FFI paths clamp oversized timeouts to one year. Swift documentation describes the catch-up ordering. ChangesAsset-lock recovery safety and readiness
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to The change improves startup recovery, but concurrent recovery attempts may still overwrite stronger asset-lock finality with an older, weaker state. Because that persisted state protects locked funds, the PR needs this race fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant AssetLockRecovery
participant TransactionBroadcaster
participant SpvRuntime
participant AssetLockStore
AssetLockRecovery->>AssetLockStore: probe local finality
AssetLockRecovery->>TransactionBroadcaster: wait_until_ready(remaining timeout)
TransactionBroadcaster->>SpvRuntime: wait_until_ready(timeout)
SpvRuntime-->>TransactionBroadcaster: readiness result
TransactionBroadcaster-->>AssetLockRecovery: readiness result
AssetLockRecovery->>TransactionBroadcaster: broadcast transaction
AssetLockRecovery->>AssetLockStore: advance status and release dispatch claim
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
🕓 Ready for review — 33 ahead in queue (commit 72129fe) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The readiness gate addresses the reported SPV startup race, but verification found three blocking regressions: an overflowing FFI timeout can abort the host, the synchronous waits can starve the executor needed to start SPV, and a delayed resume can overwrite newer asset-lock state. Two additional test gaps leave the production readiness delegation and the changed Broadcast-status path unprotected.
Source: reviewers codex general/security-auditor/rust-quality/ffi-engineer (backend model gpt-5.6-sol); final verifier codex (backend model gpt-5.6-sol). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (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),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 2 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:227: An overflowing FFI timeout can panic across the C boundary
`Instant::now() + t` panics when the resulting instant is not representable. Both public asset-lock resume FFI functions accept an unrestricted `u64` timeout and convert it directly with `Duration::from_secs`, so a value such as `UInt64.max` reaches this newly added calculation for Built or Broadcast locks. The panic occurs inside an `extern "C"` call with no unwind guard, which aborts the host instead of returning `PlatformWalletFFIResult`. Use elapsed-time subtraction or checked deadline arithmetic here, or reject unrepresentable timeout values before entering the async resume path.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:305: The readiness wait can prevent the host from ever starting SPV
This await is reached through `asset_lock_manager_catch_up_blocking`, which calls `runtime().block_on` from synchronous Swift task-group children. The Swift call site starts four such calls concurrently and explicitly documents that each parks one worker from the typical 4–8-worker cooperative pool for up to 300 seconds. With four Built locks on a four-worker pool, the new readiness waits can occupy every generic executor worker while waiting for SPV; if the host startup task has suspended between wallet loading and `startSpv`, it cannot resume to start the transport that would release those workers. Additional locks can repeat the delay as timed-out slots are replenished. Run these synchronous FFI calls on an overcommitting or dedicated blocking queue, expose a genuinely asynchronous FFI operation, or otherwise reserve executor capacity so catch-up cannot starve SPV startup.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:305-309: Revalidate the asset-lock status after waiting for readiness
`status` and `tx` are snapshotted before the readiness wait, which may now last minutes. Concurrent catch-up and explicit funding flows are supported, so another flow can advance, consume, or untrack the same lock while this task is parked. The stale Built arm then broadcasts the old transaction and `advance_asset_lock_status` unconditionally overwrites the current entry; it can downgrade a terminal Consumed tombstone to Broadcast or broadcast a transaction after a rejected build removed the row and released its input reservation. Re-read the tracked entry after readiness and make Built-to-Broadcast and final proof updates conditional on the expected current status, so terminal, removed, or newer transitions cannot be overwritten in a subsequent race window.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:336-337: Broadcast-status readiness behavior is not covered
The PR adds readiness waiting to both broadcasting arms, but the regression suite only constructs a Built lock. The Broadcast arm has materially different behavior because it swallows defensive re-broadcast errors and continues into proof waiting. Add a Broadcast-status test that verifies no re-broadcast occurs while transport is unavailable, that broadcasting begins after readiness, and that proof waiting still proceeds when the defensive re-broadcast returns an error.
In `packages/rs-platform-wallet/src/broadcaster.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/broadcaster.rs:249-250: Regression tests bypass the production SPV readiness path
The recovery tests use `StartingUpBroadcaster`, which supplies its own readiness loop and a single synthetic flag. They prove that `AssetLockManager` invokes the trait method, but they do not exercise `SpvBroadcaster` delegation or the production `SpvRuntime` predicate requiring both a started client and at least one connected peer. Removing this override or regressing either half of the production predicate would leave all new tests green. Add production-boundary coverage through a recording `SpvChannel`, plus runtime coverage for the no-client and no-peer states.
The original diff predates #4422/#4426/#4309, which rewrote both resume arms; its timeout model (None = wait forever) is itself one of the review blockers. This merge takes the v4.2-dev tree wholesale; the reimplemented transport gate lands in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cks that need broadcast
Asset-lock catch-up can run before the SPV client is started. A Built or Broadcast resume could therefore send into a transport that cannot dispatch, receive the definite pre-send Rejected verdict, and leave the lock stuck across launches.
Gate the two broadcasting resume arms on transport readiness. SpvRuntime waits for a started client with a connected peer under a 15-second ceiling, while resume_asset_lock deducts the readiness wait from any caller-supplied proof budget. Proof-only states and locally final Built or Broadcast rows bypass the transport entirely. The FFI boundary clamps extreme timeout values before they can overflow an Instant deadline.
Treat a rejected defensive send as a verdict about that attempt, not proof that an earlier send is dead. Both broadcasting arms probe local finality after a pre-dispatch rejection, keep the tracked row and reservation when the original outcome is unknown, and preserve the existing FFI error-code contract.
Serialize initial rejection cleanup against resumes with a counted ResumeDispatchClaim. A claim is releasable while the resume remains provably pre-dispatch. It becomes sticky before a broadcast can have side effects and as soon as local finality is observed, so future cancellation cannot release inputs of a possibly-live transaction. A definite pre-dispatch rejection restores ordinary RAII release, and a durable status transition away from Built clears the cleanup exclusion.
The cancellation regressions were proven red on d6adbefbbb before the lifecycle fix. cancelling_after_an_observable_resume_send_keeps_cleanup_excluded failed because cleanup returned Err(TransactionBroadcast("bad-txns-inputs-missingorspent")); cancelling_after_observing_local_proof_keeps_cleanup_excluded failed with the same unsafe definite-rejection result. Both pass after the sticky-claim change.
The proven-predispatch release exception is also behavior-sensitive: should_release_cleanup_exclusion_after_a_predispatch_resume_rejection fails if the recovery path omits release_on_drop, because cleanup remains excluded and returns TransactionBroadcastUnconfirmed; restoring the transition makes it pass.
Verification: cargo test -p platform-wallet --quiet (945 unit, 9 integration, and 4 SPV tests passed; 1 ignored); cargo test -p platform-wallet-ffi --quiet (314 unit, 26 integration, and 6 additional tests passed; 4 ignored); cargo clippy -p platform-wallet -p platform-wallet-ffi --all-features --all-targets --no-deps -- -D warnings; cargo fmt --all -- --check; git diff --check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full review |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4355 +/- ##
============================================
- Coverage 84.67% 84.62% -0.05%
============================================
Files 2786 2786
Lines 370525 372698 +2173
============================================
+ Hits 313738 315405 +1667
- Misses 56787 57293 +506
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The reimplementation fixes the timeout-overflow, production-readiness coverage, Broadcast-arm coverage, and rejected-build cleanup issues. One blocking startup limitation remains: when SPV takes longer than the hard-coded 15-second readiness window, the only automatic recovery attempt still fails before dispatch and is not retried when SPV subsequently becomes ready. The delayed recovery path also still permits a stale resume to overwrite the in-memory Consumed state, although durable-state guards and Platform's one-shot enforcement contain that issue.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 4: glm-5.3-flash (agent: phase1-reviewer, role: ffi-engineer); reviewer 5: gpt-5.6-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-5.6-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-5.6-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-5.6-sol (agent: phase2-reviewer, role: ffi-engineer); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed); agentphase1-reviewer,glm-5.3-flash— ffi-engineer (completed); agentphase1-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed); agentphase2-reviewer,gpt-5.6-sol— security-auditor (completed); agentphase2-reviewer,gpt-5.6-sol— rust-quality (completed); agentphase2-reviewer,gpt-5.6-sol— ffi-engineer (completed); agentphase2-reviewer
🔴 1 blocking | 🟡 1 suggestion(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`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:489-500: Retry recovery when readiness misses the 15-second ceiling
`await_broadcast_ready` caps a 300-second catch-up request at 15 seconds and then deliberately broadcasts even when readiness was not reached. If SPV startup or first-peer connection takes longer than that, the production broadcaster returns its pre-dispatch `Rejected` result and both Built and Broadcast recovery paths terminate without arranging another attempt. The in-tree integration automatically starts catch-up only during wallet loading and discards this ordinary failure; although the public method can be invoked manually, no readiness or reconnect event invokes it. A cold or disrupted startup that exceeds 15 seconds therefore recreates the PR's original failure: SPV can become usable immediately afterward while the transaction remains unsent until another explicit catch-up or restart. Keep the bounded wait needed by the synchronous FFI integration, but requeue unresolved locks when SPV reports readiness or otherwise retain the recovery attempt without occupying Swift's cooperative executor.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:1456-1458: Revalidate the asset-lock status after waiting for readiness
`resume_asset_lock` snapshots the transaction and status before its readiness, broadcast, and proof awaits, but the final call to `advance_asset_lock_status` replaces the live status without an expected-state check. The new dispatch claim prevents rejected-build cleanup from deleting the row, but it does not coordinate with `consume_asset_lock`. If another explicit funding flow consumes the same lock while this resume is suspended, the stale resume can replace the in-memory terminal `Consumed` tombstone with `Broadcast`, `InstantSendLocked`, or `ChainLocked` and return a proof and derivation path as though the lock remained usable. `AssetLockChangeSet::merge`, the Swift persister, and Platform's one-shot validation protect durable state and prevent a second credit, so this is not a direct fund-loss path; however, the live wallet loses its terminal classification and can attempt to reuse the spent lock until restart or reconciliation. Make the Built-to-Broadcast and final proof transitions conditional on the current status, with `Consumed` remaining absorbing, and add an interleaving test that consumes the lock while readiness or proof waiting is suspended.
| let budget = timeout.map_or(BROADCAST_TRANSPORT_READY_WAIT, |t| { | ||
| t.min(BROADCAST_TRANSPORT_READY_WAIT) | ||
| }); | ||
| let started = tokio::time::Instant::now(); | ||
| if !self.broadcaster.wait_until_ready(budget).await { | ||
| tracing::warn!( | ||
| outpoint = %out_point, | ||
| ?budget, | ||
| "resume_asset_lock: broadcast transport still not ready; \ | ||
| attempting the broadcast anyway" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Retry recovery when readiness misses the 15-second ceiling
await_broadcast_ready caps a 300-second catch-up request at 15 seconds and then deliberately broadcasts even when readiness was not reached. If SPV startup or first-peer connection takes longer than that, the production broadcaster returns its pre-dispatch Rejected result and both Built and Broadcast recovery paths terminate without arranging another attempt. The in-tree integration automatically starts catch-up only during wallet loading and discards this ordinary failure; although the public method can be invoked manually, no readiness or reconnect event invokes it. A cold or disrupted startup that exceeds 15 seconds therefore recreates the PR's original failure: SPV can become usable immediately afterward while the transaction remains unsent until another explicit catch-up or restart. Keep the bounded wait needed by the synchronous FFI integration, but requeue unresolved locks when SPV reports readiness or otherwise retain the recovery attempt without occupying Swift's cooperative executor.
source: ['claude', 'codex']
| .advance_asset_lock_status(out_point, new_status, Some(proof.clone())) | ||
| .await?; |
There was a problem hiding this comment.
🟡 Suggestion: Revalidate the asset-lock status after waiting for readiness
resume_asset_lock snapshots the transaction and status before its readiness, broadcast, and proof awaits, but the final call to advance_asset_lock_status replaces the live status without an expected-state check. The new dispatch claim prevents rejected-build cleanup from deleting the row, but it does not coordinate with consume_asset_lock. If another explicit funding flow consumes the same lock while this resume is suspended, the stale resume can replace the in-memory terminal Consumed tombstone with Broadcast, InstantSendLocked, or ChainLocked and return a proof and derivation path as though the lock remained usable. AssetLockChangeSet::merge, the Swift persister, and Platform's one-shot validation protect durable state and prevent a second credit, so this is not a direct fund-loss path; however, the live wallet loses its terminal classification and can attempt to reuse the spent lock until restart or reconciliation. Make the Built-to-Broadcast and final proof transitions conditional on the current status, with Consumed remaining absorbing, and add an interleaving test that consumes the lock while readiness or proof waiting is suspended.
source: ['claude', 'codex']
3 participants
Issue being fixed or feature implemented
The app-launch asset-lock catch-up races SPV client startup and loses permanently, stranding built-but-never-broadcast asset locks — and their funds — across every subsequent session.
Hosts drive the catch-up from wallet load:
SwiftDashSDKHost.start→PlatformWalletManager.loadFromPersistor→catchUpStuckAssetLocks, which selects every persisted lock atstatusRaw < 2(Built/Broadcast). That runs beforestartSpv— in dashwallet-ios'sSwiftDashSDKSPVCoordinator.performStart, roughly fifty lines and two setup steps before it. There is no readiness gate in between, and nothing re-invokes the catch-up on foreground, reconnect, or SPV-ready.So
resume_asset_lock'sBuiltarm broadcasts into a client that has not started. That failure is classifiedBroadcastError::Rejected— the "provably never sent" verdict — which ends the resume. The lock never leavesBuilt, so every later session repeats the identical race.Observed in a QA sim run: 60ms after the unresolved-asset-lock restore completed, the catch-up entered for a
Builtlock and failed withTransaction broadcast failed: SPV broadcast not sent: client not started. That lock held 2 DASH for days until someone retried by hand.What was done?
Gate the broadcast on transport readiness in Rust, rather than reordering each host.
TransactionBroadcastergainswait_until_ready(timeout), defaulted to "always ready" —DapiBroadcasterand all nine existing test doubles are untouched.SpvBroadcasteroverrides it, delegating to a newSpvRuntime::wait_until_readythat waits for a started client and at least one connected peer. Both halves matter: zero-peers is the other pre-sendRejectedshape at launch.resume_asset_lockawaits readiness only in the two arms that actually broadcast (Built,Broadcast), and deducts the wait from the caller's timeout so the total stays inside the requested budget.Two deliberate constraints:
InstantSendLocked/ChainLocked/RecoveredFromChainalready hold a proof and broadcast nothing. This exclusion is load-bearing rather than an optimization — four callers passtimeout: Noneon exactly that branch, so gating them would convert a cheap path re-derivation into an indefinite hang.No app-side change is required. The Swift SDK doc at the catch-up call site now states explicitly that hosts must not add a readiness gate there, since load runs before
startSpvand gating would only delay the locks that need no broadcast at all.How Has This Been Tested?
Three regression tests in
recovery.rs, driven by a broadcaster whose transport comes up partway through the test:built_resume_waits_for_the_broadcast_transport_to_come_up— under the catch-up's unbounded timeout, no broadcast attempt is recorded while the transport is down; once it comes up, the original transaction is broadcast (not a rebuild). Verified load-bearing: with the gate temporarily removed it fails on exactly the stranding condition.built_resume_bounds_the_transport_wait_by_the_caller_timeout— a transport that never comes up still fails fast with the never-sent verdict.chain_locked_resume_does_not_wait_for_the_broadcast_transport— asserts zero readiness waits and zero broadcasts.cargo test -p platform-wallet --all-targetspasses 613 + 9, exit 0.cargo clippy --all-targetsclean onplatform-walletandplatform-wallet-ffi.cargo fmt --allapplied.Breaking Changes
None.
wait_until_readyis a defaulted trait method, so external implementors ofTransactionBroadcastercompile unchanged.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes