fix(platform-wallet): finalize reconstructed asset locks as RecoveredFromChain, in-session - #4347
Merged
Merged
Conversation
…FromChain, in-session #4342's restore-scan reconstruction never actually produced its RecoveredFromChain terminal on a real restore (observed on a restored testnet wallet: all 9 reconstructed locks stuck at Broadcast for the whole session, ChainLocked after a restart — both of which hosts read as "in flight", so every historical funding tx rendered as a pending transfer): - The filter scan detects historical funding txs BEFORE any chainlock is applied, so entries insert at the pre-finality Broadcast status (the recovered_status non-final arm) rather than RecoveredFromChain. - The bulk promotion to InChainLockedBlock that the tip chainlock performs surfaces ONLY as ChainLockProcessed, which the wallet-event adapter mapped to metadata persistence alone — the promoted records never re-flowed through reconstruction, so the entries stayed pre-finality until a restart happened to re-emit them. - And when enrichment did run, it upgraded to ChainLocked, not RecoveredFromChain. Fixes: - enrich_from_record now upgrades proof-less Broadcast/IS-locked entries to RecoveredFromChain + chain proof. A lock a live flow is completing leaves that window within seconds (wait_for_proof attaches the proof via advance_asset_lock_status, which still overwrites unconditionally in the benign race), so what remains proof-less at finality is by elimination a lock nobody is completing — "final on Core, consumption unknown" is the truthful terminal. - The adapter routes ChainLockProcessed.locked_transactions through a new enrich_tracked_asset_locks_from_chain_lock: funding-family account keys are filtered lock-free, the promoted records are read back from the wallet, and the same per-record reconstruction step (extracted as apply_record) runs — so the upgrade lands in the same session, riding the same drained batch to the store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 9, 2026 00:03
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change promotes eligible pre-finality asset locks to ChangesAsset lock recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChainLockProcessed
participant CoreBridge
participant Reconstruction
participant AssetLockAdapter
ChainLockProcessed->>CoreBridge: Provide chain-lock height and locked transactions
CoreBridge->>Reconstruction: Enrich tracked asset locks
Reconstruction->>AssetLockAdapter: Persist RecoveredFromChain status and chain proof
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs`:
- Around line 242-270: Distinguish active live completion from reconstruction
before assigning RecoveredFromChain: in
packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs:242-270,
require an explicit completion-ownership or reconstruction-provenance signal in
addition to proof absence and Broadcast/InstantSendLocked status; preserve the
live-flow status at reconstruction.rs:312-312 and apply the same exclusion
during chain-lock promotion at reconstruction.rs:420-462. Document this
lifecycle signal in
packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs:64-70, and add a
regression test covering a live proof-less Broadcast entry during chain-lock
promotion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cbef70f-8ed6-494a-9a7f-2abf29665ba8
📒 Files selected for processing (4)
packages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs
Collaborator
|
🕓 Ready for review — next in queue (commit 68723cd) |
…sification Review follow-up: enrichment's "nothing live is completing it" rule is structural (proof-less + pre-finality), not provenance-based, so a chainlock promotion CAN transiently classify a still-waiting live lock RecoveredFromChain. Pin the convergence guarantee with a regression test — the live pipeline's unconditional advance_asset_lock_status overwrites the transient classification and consumption still reaches Consumed — and document the lifecycle on the variant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…layer Review follow-up (stale recovery writes vs live state): the wallet-event adapter persists enrichment snapshots from its batched drain, while live flows persist synchronously through the changeset queue — so a stale RecoveredFromChain snapshot, taken under the wallet lock before a live consumption, could land at the store AFTER the Consumed write and regress durable/host state (merge and the upserts were unconditional last-write-wins). A total status ordering would be wrong — every non-terminal transition is legitimately bidirectional (live advances overwrite RecoveredFromChain, defensive resumes re-enter Broadcast) — but Consumed is terminal, and terminality is enforceable without vetoing real transitions. Enforce it at all three write layers, making the arrival order of racing snapshots immaterial: - AssetLockChangeSet::merge skips a non-Consumed entry over a Consumed one (guards intra-batch folds); - the rs-platform-wallet-storage upsert adds a WHERE guard rejecting non-consumed over consumed; - swift-sdk persistAssetLocks skips a non-4 snapshot over a statusRaw-4 row. Tests: the exact adversarial interleaving through the real sqlite persister (Broadcast → Consumed → stale RecoveredFromChain stays Consumed; Consumed still lands over RecoveredFromChain), plus a merge unit test covering both directions and the legitimate non-terminal LWW. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4347 +/- ##
============================================
- Coverage 87.78% 86.12% -1.66%
============================================
Files 2677 2704 +27
Lines 342433 345265 +2832
============================================
- Hits 300594 297363 -3231
- Misses 41839 47902 +6063
🚀 New features to boost your workflow:
|
Its tests run in the wallet fast-path workflow (tests-rs-wallet.yml), which intentionally omits coverage upload — so on wallet-scoped PRs codecov receives no data for this crate and codecov/patch fails spuriously on any change to it (observed on #4347: 14 "missing" lines that the sqlite roundtrip suite in fact exercises). Mirrors the existing rs-platform-wallet/src ignore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/changeset/changeset.rs`:
- Around line 969-979: Preserve terminal Consumed asset locks across merges and
persistence: in AssetLockChangeSet::merge at
packages/rs-platform-wallet/src/changeset/changeset.rs:969-979, remove
tombstones for effective Consumed entries and clear older tombstones when a
newer Consumed entry arrives; update the delete statement at
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:53-54 to
avoid deleting rows whose stored status is consumed; add the
Consumed-then-removed roundtrip assertion at
packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs:586-620;
and in PlatformWalletPersistenceHandler at
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:260-272,
skip removal when PersistentAssetLock.statusRaw is 4.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ff63c42-d6a2-4231-b990-3939d3f657ad
📒 Files selected for processing (5)
.codecov.ymlpackages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rspackages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
Review follow-up: the Consumed-is-terminal guards covered stale upserts but not stale `removed` tombstones — merge could retain a Consumed entry alongside a tombstone, and both stores apply upserts before removals, so the delete would win. The only removal emitter (untrack_asset_lock) fires exclusively for Built rows whose broadcast was definitively rejected, so a Consumed/removed pair for one outpoint has no legitimate producer; this is defense in depth matching the upsert guard, and consistent with Consumed rows being deliberately retained for historical lookup. - merge: a Consumed entry clears an earlier-folded tombstone, and an incoming tombstone is dropped when the effective entry is Consumed; - sqlite delete gains `AND status != 'consumed'`; - swift persistAssetLocks skips deleting a statusRaw-4 row; - tests: merge covers all three tombstone directions, and the sqlite interleaving test adds stale-removal-after-Consumed (row survives) plus the legitimate rejected-Built removal (row deletes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
added a commit
to dashpay/dashwallet-ios
that referenced
this pull request
Aug 9, 2026
…e pin The "Platform release pin" bullet still named v4.1-dev and the long-gone local/tx-decode-plus branch. State how pinning actually works (the ../platform sibling checkout's branch, consumed by path) and the real release floor: v4.2-dev at or past dashpay/platform#4347, without which restored wallets render every historical funding tx as a pending transfer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 9, 2026
…eam asset-lock substrate: dashpay#4342 restore-time reconstruction, dashpay#4346 proof-blob validation, dashpay#4347 RecoveredFromChain finalization) Ports three merged upstream commits that rework the asset-lock substrate, plus the call-site adaptation qa5's richer 7-arg builder signature needs. Verified green on this branch: platform-wallet 635 passed, 0 failed. dashpay#4342's only conflict was changeset/core_bridge.rs, where qa5 carried the watermark-fault logic inline and upstream had extracted it into commit_batch + BatchDiagnostics. Resolved toward upstream: it preserves the freeze guard, the is_empty_no_records skip and the SYNC WATERMARK FROZEN marker, and additionally fixes an accounting bug qa5 had — a height was counted persisted before store() rather than only in the Ok arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Follow-up to #4342: its restore-scan reconstruction never actually produced the
RecoveredFromChainterminal on a real restore. Observed on a restored testnet wallet (dashwallet-ios): all 9 reconstructed locks sat atBroadcastfor the whole session and only reachedChainLockedafter a restart — both inside the pending window every host reads as "in flight", so every historical funding tx rendered as a pending transfer.Why it happened
Broadcaststatus (recovered_status's non-final arm) — never atRecoveredFromChain.InBlock → InChainLockedBlockpromotion the tip chainlock performs surfaces only asChainLockProcessed, which the wallet-event adapter mapped to metadata persistence alone. The promoted records never re-flow throughTransactionDetected/BlockProcessed, so reconstruction/enrichment never saw them.ChainLocked, notRecoveredFromChain.Fixes
enrich_from_recordupgrades toRecoveredFromChain(+ chain proof). A lock a live flow is completing leaves the proof-less Broadcast/IS-locked window within seconds (wait_for_proofattaches the proof viaadvance_asset_lock_status, which still overwrites unconditionally in the benign race) — what remains proof-less at finality is by elimination a lock nobody is completing, and "final on Core, consumption unknown" is the truthful terminal.ChainLockProcessedthrough a newenrich_tracked_asset_locks_from_chain_lock. Deliberately record-free: under the defaultkeep-finalized-transactions=OFFthe promotion evicts the records and the event retains only txids, so the hook upgrades the tracked entries directly by txid and builds the chain proof from the chainlock's own height (the same fact the resume path's CL-from-metadata fallback relies on). Funding-family filtering happens on the event's account-type keys, so chainlocks promoting plain payments never take the wallet-manager write lock.resume_asset_lockno longer downgrades aRecoveredFromChainentry into the pending window: a resume proves nothing new about Platform-side consumption. Without this, the launch-timecatchUpStuckAssetLockssweep (or any explicit resume that doesn't end in a spend) silently resurrected the false-"Pending" state.Verification
cargo test -p platform-wallet --lib: 601/601 (new tests: chainlock-promotion upgrade at reconstruction and bridge level; resume keepsRecoveredFromChain; existing enrich/resume tests updated to the new terminals).dashpaybuild): fresh wipe-and-recover scan inserted all 9 historical locks atBroadcast, and the first post-sync chainlock upgraded all 9 tostatusRaw 5+ proof in the same session —attaching chain proof to tracked asset lock from chainlock promotion×9 in the SDK log, funding types classified (identity registration / platform / shielded), and the host app renders them outside both pending and consumed.Known residual (narrow): a foreground
catchUpStuckAssetLockssweep that fires mid-restore-scan can still resume a just-reconstructedBroadcastrow toChainLocked+proof before the chainlock arm reaches it, parking that row at 3. The window is the few minutes between insert and the next chainlock, only on a restore in progress.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests