fix(platform-wallet): close the asset-lock resume broadcast race - #4636
Open
shumkov wants to merge 4 commits into
Open
fix(platform-wallet): close the asset-lock resume broadcast race#4636shumkov wants to merge 4 commits into
shumkov 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.
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
September 9, 2026 06:20
Contributor
|
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 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. 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
🚥 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 |
Collaborator
|
✅ 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
approved these changes
Sep 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
The asset-lock resume path broadcasts a
Builtrow before advancing it toBroadcast. For the whole in-flight broadcast the row still readsBuilt, so aconcurrent create-path definite
Rejecteduntracks it — the untrack guard spares onlyrows already past
Built— and releases its funding reservation, while the resume'sbroadcast of the same transaction may still deliver. A later build can then reselect
those inputs and produce a self-conflicting transaction.
This re-derives the fix from #4016 on the current module layout, per the rework
directive on that PR. #4016 has been dormant since 2026-08-07; its branch predates the
asset_lock/sync/split and conflicts throughout, so this is a fresh implementationrather than a rebase. Supersedes #4016.
What was done?
promote_built_to_broadcastCAS in the tracking module: promotes only while therow still reads
Built, under the write lock, otherwise returnsAlreadyAdvanced { status, proof }. The changeset is enqueued following the existingconsume_asset_lockpattern.Builtarm CAS-promotes before broadcasting and re-dispatches fromthe returned status/proof on
AlreadyAdvanced. TheMaybeSentbounded-wait mappingis unchanged. On a definite
Rejectedthe row is left atBroadcast; the Broadcastarm's defensive re-broadcast preserves resumability. No rollback to
Built— anearlier incarnation of this fix used one, and an unowned rollback can clobber a
concurrent successful resume.
concurrently finalized row cannot be downgraded.
to route later resumes down the Broadcast arm, which returned
TransactionBroadcastUnconfirmedand never consultedinput_conflict. That arm nowpreserves a sighted conflict through the bounded proof wait, so an offline relaunch
reproduces
AssetLockInputContestedon every pass rather than only the first.Broadcastfrom "was broadcast once"to "a broadcast was attempted"; every comment that leaned on the old meaning is
reworded, including the contested-verdict docs in the Rust error type and both mobile
SDKs.
Tests
Test would have caught this in CI: ✖ before the fix, ✔ after.
rejected_create_while_resume_broadcasts_keeps_row_and_reservation— the forwardinterleaving, which had no coverage. Verified RED on the parent by applying only the
test hunks: it fails at the ordering assertion, and with those assertions stripped it
still fails on the symptom (
TransactionBroadcast— row untracked, reservationreleased mid-flight).
create_broadcast_does_not_downgrade_a_concurrently_finalized_row— RED on theparent.
Built" is updated: that assertion is invalidated by design, and it now assertsthe row stays tracked at
Broadcastand resumable via the Broadcast arm.Broadcast.cargo test -p platform-walletand-p platform-wallet-ffipass; clippy-D warningsandfmt --checkclean.Explicitly out of scope, and deliberately not ported from #4016:
status_persist_serial, manager retirement, generation gates. That PR grew from atwo-commit fix into a wallet-lifecycle project whose own machinery kept generating
blockers; this keeps to the race.
Summary by CodeRabbit
Bug Fixes
Documentation