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>
4 tasks
shumkov
added a commit
to HashEngineering/platform
that referenced
this pull request
Sep 1, 2026
…tlin heals Addresses the review on dashpay#4336. Swift load-path heal (blocking finding). The callback-time reconcile in `persistAssetLocks` only fires when a lock upserts again, and `Consumed` is terminal — it never upserts again. A wallet whose lock finalized before that reconcile existed therefore has no future callback to repair its funding TXOs, and `loadWalletList`'s `isSpent == false` fetch handed them back to Rust as spendable on every launch. `loadWalletList` now collects the funding txids of every lock at `InstantSendLocked` or beyond, drops the TXOs those locks consumed from the restore set, and saves the repaired flags opportunistically (log + rollback on failure, the `healIdentityIsLocalFlags` pattern). Exclusion happens before the save, so it does not depend on the repair becoming durable. Non-clobbering, non-fatal heal writes. Both Kotlin heals were read-then-upsert round trips that wrote the whole row back from an in-memory copy. They are now column-scoped `UPDATE … SET isSpent = 1 … WHERE isSpent = 0` DAO queries: they cannot regress an already-spent row and cannot write back stale copies of columns they do not own. Vout-independent Kotlin finality lookup. The load guard probed the synthetic `<spending txid>:0` outpoint, but DIP-0027 lets one funding transaction carry several credit outputs and `wallet/asset_lock/sync/reconstruction.rs` persists each tracked lock under its own credit-output index — a valid lock can live at `:1` with no `:0` row at all, and the guard missed it. Replaced with `AssetLockDao.maxStatusForTxid`, the same guarded 64-hex `<txid>:` prefix match `fundingTypeForTxid` already uses, reduced with `MAX`: finality belongs to the transaction, so any of its locks reaching `InstantSendLocked` means the inputs are gone. One explicit failure policy for that lookup. It is the only new read in `buildUtxoRestoreData`, it runs under `guardedLoad(emptyArray())`, and the Android load surface is array-only — an escaping failure returned a SUCCESSFUL EMPTY restore, which Rust reads as a fresh coinless device for every wallet. `spendByFinalizedAssetLock` now answers `true` / `false` / `null`, where `null` means "could not read": the caller drops that one candidate and never heals it (nothing was proven), and every unrelated wallet, account and TXO still restores. Under-reporting one output for a launch is recoverable and non-durable; handing a consumed output back as spendable is what the guard exists to stop. Swift error propagation. Two load-bearing `try?`s failed OPEN, each in the direction that defeats its own guard: - the callback-time reconcile treated a stale-TXO fetch failure as "no rows to heal" and the shim returned 0, so a terminal `Consumed` status could commit while its funding TXOs stayed `isSpent == false` with no future upsert to repair them. `persistAssetLocks` is now result-bearing and `persistAssetLocksCallback` returns nonzero, which fails the Rust round and rolls the changeset back. - `finalizedAssetLockFundingTxids` mapped a thrown fetch to an empty set, and the caller reads an empty set as the positive claim "no lock has finalized" — restoring exactly the phantom inputs the guard withholds. It now throws, and `loadWalletList` returns `(nil, 0, true)` like the adjacent unspent fetch. Rust regression drives the production accessor. The previous version asserted on `computed_core_balance` directly, so it never called `account_balances_blocking` and stayed green against the pre-fix cached read. `account_balances_blocking_ignores_the_stale_cache` now builds a `PlatformWalletManager` over the funded fixture and asserts the returned `AccountBalanceRow` buckets in three steps: reproduce a freshly updated non-empty balance, track a live confirmed→locked reclassification the cache has not seen, then track the drain to zero. The direct-fold assertions remain as `computed_core_balance_matches_update_balance_bucket_for_bucket`, a bucket-policy companion (the fold duplicates `ManagedCoreFundsAccount::update_balance` and nothing in the type system keeps the two in step). Regression coverage, red before / green after. - `account_balances_blocking_ignores_the_stale_cache` (Rust): FAIL with line 432 reverted to `.map(|a| a.balance)` — `(10000000,0,0,0)` vs the expected `(3000000,0,0,7000000)` — and FAIL against a `WalletCoreBalance::default()` helper stub; PASS as written. - `loadSkipsATxoConsumedByAFinalizedAssetLockPersistedAtANonZeroVout` (Kotlin): FAIL with the vout-0 `getByOutPointHex` probe restored; PASS with the txid-prefix lookup. - `aFailingFinalizedLockLookupDropsOnlyItsOwnCandidate` (Kotlin): a Room open-helper factory faults the single `SELECT MAX(statusRaw) FROM asset_locks` statement — not the table, which two other restore builders also read — and asserts an unrelated TXO on the same wallet and an unrelated wallet's TXO both still restore, and that the unanswered candidate is dropped unhealed. FAIL with the failure allowed to escape (the whole restore comes back empty); PASS as written. - `AssetLockSpendVisibilityTests` (Swift, new): the SwiftData coverage both heals were missing — load excludes and heals a TXO consumed by a finalized lock, does so for a lock persisted at a non-zero vout, and the callback reconcile flips the linked TXO and reports success. Each FAILs with its guard disabled; all PASS as written. - `testPersistAssetLocksFailsTheRoundWhenTheStaleTxoFetchThrows` (Swift): faults only the `PersistentTxo` read, then asserts `persistAssetLocks` reports failure, the round closed as failed, and no `PersistentAssetLock` row committed. FAIL with the catch reverted to log-and-continue — the round reports success, commits, and lands the terminal status over funding TXOs still `isSpent == false`; PASS as written. - `testLoadReportsFailureWhenTheFinalizedLockFetchThrows` (Swift): faults only the `PersistentAssetLock` read, asserts the seam observed `PersistentWallet, PersistentTxo, PersistentAssetLock` (the earlier reads were served, so the failure is the lock read alone), then `errored == true` with nil entries and count 0. FAIL with the fetch reverted to `try? … ?? []` — the load returns a successful one-entry restore carrying the phantom UTXO; PASS as written. Also names the Swift `InstantSendLocked` threshold instead of a bare `2`, mirroring Kotlin, and corrects both status docs to list `RecoveredFromChain` (5) — added by dashpay#4347 after this branch forked. Fault-injection seam for the Swift fail-closed reads. Both Swift guards only take their failure branch when a `fetch` throws, which a live store never does on demand, and store-level corruption is no substitute: it trips `loadWalletList`'s earlier wallet/TXO fetches too, so a load test built on it would pass for the wrong reason. The reads whose failure must reject the round therefore go through `ModelFetching`, a handler-owned one-method protocol whose production implementation is `ModelContext.fetch` verbatim. A test injects a fetcher that faults one model type and serves every other read live, so each regression pins its own fetch — the load test asserts the wallet and unspent-TXO reads were served before the lock read threw. Verified: `cargo nextest run -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-features --locked -E 'not test(~shield)'` 1580 passed; `cargo test -p platform-wallet --lib` 890 passed; `cargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi -p rs-unified-sdk-jni --all-features --all-targets` clean for these crates; `cargo fmt --all --check` clean; `:sdk:testDebugUnitTest :sdk:testReleaseUnitTest :sdk:compileDebugAndroidTestKotlin` green; `xcodebuild test -scheme SwiftDashSDK -only-testing:SwiftDashSDKTests` on an iOS 26 simulator, 391 tests, only the two pre-existing `KeychainSignerAdditionalSigningKeys` keychain-environment failures (identical with this branch's Swift changes reverted). `rs-drive`'s unused `DocumentPropertyType` import (a7ccac2) makes a workspace-wide `-D warnings` clippy fail on unmodified code — pre-existing, not from here. Co-Authored-By: Claude Opus 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