fix(platform-wallet): survive an ambiguous re-broadcast when resuming a Built asset lock - #4367
Conversation
… a Built asset lock Extracted from #4337; its already-consumed half is superseded by the merged #4357. The Built arm of resume_asset_lock propagated every broadcast error, including MaybeSent. For a lock stuck at Built whose transaction WAS broadcast (the app died between the send and the status advance), MaybeSent is the expected answer on every retry — the network already holds the tx, so each re-broadcast draws the same ambiguity. The resume failed, the lock stayed Built, every recovery pass repeated the same abort, and the top-up never completed. Observed live on an Android testnet wallet. Only a definite Rejected now stops the resume; MaybeSent advances the lock to Broadcast and proceeds to the proof wait, matching what the Broadcast arm already does with the identical signal and keeping a genuinely un-broadcast tx resumable at Built. Tests: two regression tests covering the ambiguous and definite branches, asserting the status transition as well as the error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — 1 ahead in queue (commit 6d4bebd) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAsset-lock recovery now distinguishes ambiguous ChangesAsset-lock recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Recovery
participant Broadcaster
participant ProofWait
participant LockTracker
Recovery->>Broadcaster: Resume Built lock
Broadcaster-->>Recovery: MaybeSent
Recovery->>LockTracker: Set lock to Broadcast
Recovery->>ProofWait: Wait for proof
ProofWait-->>LockTracker: Proof wait timeout
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Extracts the still-needed half of #4337 onto current
v4.2-dev(authorship preserved — the commit is @HashEngineering's, adapted from #4337's head).Why #4337 was split
#4337 fixed two independent ways an asset-lock top-up could never finish. Its first half — classifying Platform's "already completely used" rejection and settling the tracked lock — is superseded by #4357 (merged 2026-08-11), whose shared
reconcile_asset_lock_submit_resultcovers the same rejection across all four funded flows with outpoint binding and a safer policy: the unauthenticated verdict never tombstones the lock; it is preserved asRecoveredFromChain/ consumption-unknown with a ChainLock proof, surfacing the same typed code 24.This PR carries the second half, which #4357 does not touch.
The remaining bug
The
Builtarm ofresume_asset_lockpropagated every broadcast error, includingMaybeSent. For a lock stuck atBuiltwhose transaction was broadcast — the app died between the send and the status advance —MaybeSentis the expected answer on every retry: the network already holds the tx, so each re-broadcast draws the same ambiguity. The resume failed, the lock stayedBuilt, every recovery pass repeated the same abort, and the top-up never completed. Observed live on an Android testnet wallet (the same incident that produced #4337).The fix
Only a definite
Rejected(provably never entered the send pipeline) stops the resume.MaybeSentlogs, advances the lock toBroadcast, and proceeds to the proof wait — exactly what theBroadcastarm already does with the identical signal. A genuinely un-broadcast tx still fails definitively and stays resumable atBuilt.Two regression tests cover the ambiguous and definite branches, asserting the resulting lock status as well as the error (
built_resume_survives_an_ambiguous_rebroadcast_and_advances,built_resume_still_fails_on_a_definite_rejection).Note for reviewers: #4355 (open) touches the same
Builtarm to wait for SPV transport before this broadcast; the changes compose but whichever lands second needs a trivial rebase.Verification
cargo test -p platform-wallet --lib— 654 passed, 0 failed (both new tests included)cargo clippy -p platform-wallet --all-targets— no new warnings (1 pre-existing on the unmodified base)cargo fmt --all— clean🤖 Generated with Claude Code
Summary by CodeRabbit