fix(platform-wallet): fall back to persister for chainlocked asset-lock tx records#3619
Conversation
…ck tx records
Five asset-lock proof lookup sites in
`wallet/asset_lock/sync/{proof,recovery}.rs` resolve a transaction
record from the in-memory `transactions()` map keyed by
`(account_index, txid)`. With upstream's `keep-finalized-transactions`
Cargo feature OFF (the default), chainlocked records are evicted from
that map and only their txids retained in `finalized_txids` for dedup.
The chain-lock height / block hash that an asset-lock proof needs is
no longer reachable through the wallet-info API.
The narrow failure mode is "first lookup happens after the
chainlock-eviction window" — `wait_for_proof` polls and normally
observes `InstantSend` / `InBlock` state while the record is still
present, but a tx that's already chain-locked at the moment the proof
flow starts (e.g. crash recovery resuming an asset lock that
chain-locked while we were down) hits the missing-record path.
This adds a persister fallback. The persister received the full record
on the chainlock-transition `store` call before eviction, so it can
answer the lookup.
Changes:
* `PlatformWalletPersistence::get_core_tx_record(wallet_id, txid) ->
Result<Option<TransactionRecord>, PersistenceError>` — new trait
method with a default `Ok(None)` impl. Backward-compatible:
persisters that don't index records by txid (the in-tree
`NoPlatformPersistence`, no-op test stubs in the wider tree) need
no change. Persisters whose backing store keys records by txid
(`SqliteWalletPersister` in evo-tool, the SwiftData iOS persister)
should override; until then those callers see the same behavior as
before this PR (None response, caller's existing not-found path
applies).
* `WalletPersister::get_core_tx_record(txid)` — `pub(crate)` per-wallet
passthrough.
* `record_or_persister(in_memory, persister, txid) -> Option<TransactionRecord>`
— `pub(super)` helper in `proof.rs` that returns the in-memory
record if present, otherwise queries the persister. Persister
errors are logged at `debug` and surfaced as `None` so a transient
backend failure doesn't turn into a hard error in the proof flow.
* Five lookup sites switched to the helper:
- `validate_or_upgrade_proof` (proof.rs): error message broadened to
"in-memory or persister".
- `upgrade_to_chain_lock_proof` (proof.rs): "tx not found" no longer
errors; falls through to `wait_for_chain_lock` so SPV events can
still complete the proof.
- `wait_for_chain_lock` (proof.rs): poll loop now also checks
persister so a chainlock that arrives between polls and is
evicted is still observed on the next iteration.
- `wait_for_proof` (proof.rs): same treatment.
- `resolve_status_from_info` (recovery.rs): converted from
associated `fn` → method (`&self`) to access the persister.
* Tests: 5 new unit tests in `proof.rs::tests` covering the helper —
in-memory hit short-circuits, persister-only hit returns the
record, both-miss returns `None`, default trait impl returns `None`,
backend errors swallowed as `None`. Fake `PlatformWalletPersistence`
implementations (`FakeRecordStore`, `ErroringStore`) added inline.
Total: 120/120 platform-wallet lib tests pass.
Backlinks #3617 (CodeRabbit's "Major" finding on the
proof.rs eviction window).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 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 |
Review GateCommit:
|
…llback Hooks the persister-fallback path added in the previous commit through to the iOS persister so it actually fires in production builds. The trait method `PlatformWalletPersistence::get_core_tx_record` is the seam; this commit adds the FFI plumbing + the Swift implementation that backs it via the existing `PersistentTransaction` SwiftData rows. ## platform-wallet (trait doc) Tightened the field contract on `get_core_tx_record`. Implementations are only required to populate `txid` + `context` (and the `BlockInfo` inside `InChainLockedBlock` / `InBlock` carrying real height + hash + timestamp). Other fields MAY be best-effort placeholders; callers (currently only the asset-lock proof flow) MUST NOT read them. This makes the FFI-backed implementation feasible without serializing the full `Transaction` body across the C ABI. ## platform-wallet-ffi * `PersistenceCallbacks.on_get_chainlocked_tx_record_fn` — new C callback. Takes a wallet_id + txid; on hit, populates `out_block_height`, `out_block_hash` (32 bytes), `out_block_timestamp` and sets `out_found = true`. Returns 0 on success regardless of hit / miss; non-zero is treated as a transient backend failure (surfaced as `None` per the helper contract). Scope is intentionally narrow: chainlocked records only — IS-locked / mempool / InBlock records aren't evicted from the in-memory map and so don't need a fallback. * `FFIPersister::get_core_tx_record` — overrides the trait default to call the new callback and synthesize a minimal `TransactionRecord` with `context = InChainLockedBlock(BlockInfo::new(height, hash, timestamp))`. Placeholder transaction body / account_type / direction / etc. — proof flow callers never read those, per the trait contract. ## swift-sdk * `PlatformWalletPersistenceHandler.chainlockedTxRecord(walletId:txid:)` — internal lookup. Walks `PersistentTransaction` rows by `txid` (raw 32 wire bytes — same orientation Rust hands across the FFI) filtered to `context == 3` (InChainLockedBlock). Returns nil on miss or when the row's `blockHash` is missing / wrong-length (treated as miss rather than fabricating a zero hash that would round-trip to Rust as a real block id). * `getChainlockedTxRecordCallback` — top-level C shim mirroring the pattern of the other persistence callbacks. Handles the Rust→Swift→Rust conversion + populates the output pointers. * `makeCallbacks()` registers the new shim. Existing `PlatformWalletManager()` construction picks it up automatically; the example app needed no changes. ## Verification - `cargo check --workspace --all-targets`: clean - `cargo test -p platform-wallet --lib`: 120/120 (the 5 new helper tests from the prior commit still pass since the helper itself is unchanged) - `cargo clippy -p platform-wallet-ffi --all-targets`: clean (the 2 warnings are pre-existing in unrelated test code) - `bash packages/swift-sdk/build_ios.sh --target sim --profile dev`: BUILD SUCCEEDED end-to-end including SwiftExampleApp link Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rest of `makeCallbacks()` is one-line-per-registration; the new line follows the same shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "35b1ecf7baf930357d5c45653ca12438079f24f802e7686d752c183af634d520"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The persister fallback is a good narrow fix for chainlocked asset-lock records that have been evicted from the in-memory wallet map, and the helper itself is covered by focused unit tests. I did not find a consensus or safety blocker. The remaining review points are follow-through/behavioral: the in-tree FFI/Swift persistence surface still cannot answer the new lookup, the missing-record path in upgrade_to_chain_lock_proof now waits for timeout instead of failing fast, and one recovery callsite hides backend errors / does persister I/O while holding the wallet-manager write lock.
Reviewed commit: 757f035
🟡 3 suggestion(s) | 💬 1 nitpick(s)
GitHub rejected the inline review payload for this run (HTTP 422), so I’m posting the verified findings as a top-level review rather than dropping the dispatcher-required review.
🟡 suggestion: FFI / Swift persister does not yet override `get_core_tx_record` — iOS recovery path remains inert
packages/rs-platform-wallet-ffi/src/persistence.rs:63-312
The new PlatformWalletPersistence::get_core_tx_record lookup is the entire point of this PR's chainlock-eviction recovery, but FFIPersister (the only in-tree persister used by Swift/iOS, at packages/rs-platform-wallet-ffi/src/persistence.rs:312) does not override it and the C PersistenceCallbacks vtable exposes no lookup hook. Result: every FFI/Swift-backed wallet inherits the trait's default Ok(None), so once the in-memory transactions() map evicts a chainlocked record, all five updated asset-lock call sites still resolve to None and the chainlock recovery path the PR adds is unreachable on iOS.
The PR description acknowledges this and points to follow-up PRs for SqliteWalletPersister and the SwiftData persister, so this is intentional groundwork rather than a defect — flagging because the in-tree FFI surface needs a paired callback (and FFIPersister impl) before this PR's intent applies to the iOS build, and the Ok(None) default gives no compile-time signal driving that follow-through. Until then, the only behavior change visible to FFI consumers is the fast-fail → full-timeout regression noted separately on upgrade_to_chain_lock_proof.
🟡 suggestion: `upgrade_to_chain_lock_proof`: missing record now waits the full timeout instead of erroring fast
packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:185-204
Previously this function returned AssetLockProofWait("Transaction X not found in account Y") immediately when the in-memory lookup missed. After the change, a miss yields in_memory = None; if the persister also returns None (which is the case for any backend that hasn't overridden get_core_tx_record — i.e. all in-tree backends today), record_or_persister returns None, the match falls to None => None, height becomes None, and the code dispatches to wait_for_chain_lock(...) which polls until height_timeout elapses and errors with FinalityTimeout.
Because the function already validated tracked_asset_locks.get(out_point) above, the asset lock is known-tracked, so silently waiting is mostly consistent with this function's documented "otherwise wait" semantics. But it does change the fast-fail behavior for genuinely-unknown txids (e.g. wallet-state mismatch, post-wipe race) and — combined with the FFI gap above — means production iOS callers always wait the full chainlock timeout instead of recovering. Either preserve a fast-fail when both lookups miss, or document the timeout-on-miss behavior on the function so callers size timeouts accordingly.
🟡 suggestion: Recovery path silently downgrades status on transient persister error
packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:100-115
record_or_persister logs persister errors at debug and surfaces them as None, which is the right call inside the wait_for_chain_lock / wait_for_proof poll loops (the next tick retries). But resolve_status_from_info is a one-shot recovery call: a transient backend error here makes the lock silently classify as AssetLockStatus::Broadcast even when the underlying tx is actually chain-locked. The follow-up resume_asset_lock then takes the wasteful Broadcast → wait_for_proof path instead of constructing a ChainAssetLockProof directly. The user eventually recovers, but the failure is invisible to operators. Either log at warn/error from this call site, or distinguish "persister returned None" from "persister failed" in resolve_status_from_info so a real backend error is visible.
💬 nitpick: `recover_asset_lock_blocking` now holds the wallet manager write lock across persister I/O
packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:47-89
recover_asset_lock_blocking acquires self.wallet_manager.blocking_write() and then calls self.resolve_status_from_info(&info.core_wallet, ...). After this PR, resolve_status_from_info can fall through to record_or_persister, which performs a synchronous self.persister.get_core_tx_record(...) lookup on miss — disk I/O, FFI callbacks, or arbitrary backend logic, now all serialized behind the wallet-manager write lock. Consider snapshotting the in-memory result, dropping wm, then doing the persister fallback (and, if needed, re-acquiring) so the lock-hold time stays bounded to the in-memory lookup as before.
🤖 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.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:63: FFI / Swift persister does not yet override `get_core_tx_record` — iOS recovery path remains inert
The new `PlatformWalletPersistence::get_core_tx_record` lookup is the entire point of this PR's chainlock-eviction recovery, but `FFIPersister` (the only in-tree persister used by Swift/iOS, at packages/rs-platform-wallet-ffi/src/persistence.rs:312) does not override it and the C `PersistenceCallbacks` vtable exposes no lookup hook. Result: every FFI/Swift-backed wallet inherits the trait's default `Ok(None)`, so once the in-memory `transactions()` map evicts a chainlocked record, all five updated asset-lock call sites still resolve to `None` and the chainlock recovery path the PR adds is unreachable on iOS. The PR description acknowledges this and points to follow-up PRs for `SqliteWalletPersister` and the SwiftData persister, so this is intentional groundwork rather than a defect — flagging because the in-tree FFI surface needs a paired callback (and `FFIPersister` impl) before this PR's intent applies to the iOS build, and the `Ok(None)` default gives no compile-time signal driving that follow-through. Until then, the only behavior change visible to FFI consumers is the fast-fail → full-timeout regression noted separately on `upgrade_to_chain_lock_proof`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:185: `upgrade_to_chain_lock_proof`: missing record now waits the full timeout instead of erroring fast
Previously this function returned `AssetLockProofWait("Transaction X not found in account Y")` immediately when the in-memory lookup missed. After the change, a miss yields `in_memory = None`; if the persister also returns `None` (which is the case for any backend that hasn't overridden `get_core_tx_record` — i.e. all in-tree backends today), `record_or_persister` returns `None`, the match falls to `None => None`, `height` becomes `None`, and the code dispatches to `wait_for_chain_lock(...)` which polls until `height_timeout` elapses and errors with `FinalityTimeout`. Because the function already validated `tracked_asset_locks.get(out_point)` above, the asset lock is known-tracked, so silently waiting is mostly consistent with this function's documented "otherwise wait" semantics. But it does change the fast-fail behavior for genuinely-unknown txids (e.g. wallet-state mismatch, post-wipe race) and — combined with the FFI gap above — means production iOS callers always wait the full chainlock timeout instead of recovering. Either preserve a fast-fail when both lookups miss, or document the timeout-on-miss behavior on the function so callers size timeouts accordingly.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:100: Recovery path silently downgrades status on transient persister error
`record_or_persister` logs persister errors at `debug` and surfaces them as `None`, which is the right call inside the `wait_for_chain_lock` / `wait_for_proof` poll loops (the next tick retries). But `resolve_status_from_info` is a one-shot recovery call: a transient backend error here makes the lock silently classify as `AssetLockStatus::Broadcast` even when the underlying tx is actually chain-locked. The follow-up `resume_asset_lock` then takes the wasteful `Broadcast → wait_for_proof` path instead of constructing a `ChainAssetLockProof` directly. The user eventually recovers, but the failure is invisible to operators. Either log at `warn`/`error` from this call site, or distinguish "persister returned None" from "persister failed" in `resolve_status_from_info` so a real backend error is visible.
…ody in persister fallback
Two fixes on top of the previous commit:
1. **FFI now sends the actual context kind, Rust faithfully
reconstructs.** Previously the FFI was scoped to "chainlocked
only" and the Rust side wrapped every hit as
`InChainLockedBlock(...)` regardless. The Swift side compensated
by filtering rows to `context == 3` in the predicate. That was
correct in steady state (the eviction invariant means
persister-only-not-memory implies chainlocked) but wrong at
startup: an in-memory map starts empty, so any persister hit on
a non-chainlocked row would round-trip to Rust as a false-
positive chainlock — `resolve_status_from_info` would emit
`(ChainLocked, ChainAssetLockProof)` for a tx that isn't
chainlocked yet.
Now the FFI carries `out_context_kind` (0=Mempool, 1=IS,
2=InBlock, 3=InChainLockedBlock). Rust constructs the matching
`TransactionContext` variant, the chainlock filter on Swift
goes away, and false-positive chainlocks are impossible. IS
hits surface as `None` to the proof flow (the persister
doesn't store the IS-lock blob; SPV-event wait completes the
proof from the live stream — same outcome as if the fallback
weren't wired at all).
2. **Persister returns the real transaction bytes; Rust decodes a
real `Transaction`.** Previously the synthesized
`TransactionRecord` carried a placeholder `Transaction { version:
1, lock_time: 0, input: [], output: [], … }` because the FFI
surface didn't carry the tx body. The proof flow doesn't read
`record.transaction` today, so the placeholder was technically
safe — but it was a lie that would silently corrupt any future
code reading those fields.
`PersistentTransaction.transactionData` is what gets stored on
the FFI write path (consensus-encoded — `dashcore::consensus::
encode::serialize` upstream, NOT bincode). The new callback
hands those bytes back over FFI; Rust decodes via
`Transaction::consensus_decode`. Buffer ownership: Swift
allocates with `UnsafeMutablePointer<UInt8>.allocate`, Rust
takes the pointer, an RAII guard on the Rust side calls the
paired `on_get_core_tx_record_free_fn` exactly once on every
exit path (success, decode failure, IS-skip, unknown context
kind).
`PersistentTransaction.transactionData` is now non-optional —
the FFI write path always populates it in practice (the `Data?`
was a vestige of a defensive guard that never fires). Init
gains a required `transactionData` arg; the upsert path lifts
the bytes extraction to a single binding shared between the
init and the unconditional post-init assignment. The one stub
creation site (UTXO upsert path that pre-creates the parent
transaction row before the real upsert arrives) passes empty
`Data()` — the real upsert overwrites every field including
`transactionData`; an orphaned stub correctly reads back as
miss because the empty buffer won't decode.
Touched the example app's `StorageRecordDetailViews` (one line) to
drop the `?.count` optional chain on the de-optionalized field.
Verification:
- `cargo check --workspace --all-targets`: clean
- `cargo test -p platform-wallet --lib`: 120/120
- `cargo clippy -p platform-wallet-ffi --all-targets`: clean
- `bash packages/swift-sdk/build_ios.sh --target sim --profile dev`:
BUILD SUCCEEDED end-to-end including SwiftExampleApp link
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… drop lock before persister I/O Addresses three findings from the @thepastaclaw review on commit 757f035 (the FFI/Swift wiring + tx-bytes plumbing in d8de2c2 and 8e9b75d already addressed the fourth — "FFI not wired" — and already reach the iOS build through the new `on_get_core_tx_record_fn` callback). ## #2 — fast-fail regression in `upgrade_to_chain_lock_proof` Pre-PR, a missing in-memory record errored fast with `AssetLockProofWait("Transaction X not found in account Y")`. After the helper landed, the missing case fell through to `wait_for_chain_lock(...)` and burned the full chainlock timeout before returning `FinalityTimeout` — a regression for genuine "tx never tracked" cases (wallet-state mismatch, post-wipe race). Restored the fast-fail: when both the in-memory map and the persister miss, the function returns the same `AssetLockProofWait` error pre-PR callers expected. The "persister returned a non-chainlocked record" case still falls through to `wait_for_chain_lock` (correct — the tx exists, just hasn't chainlocked yet). ## #3 — silent error downgrade on transient persister failure `record_or_persister` previously logged backend errors at `debug` and surfaced them as `None`. That's correct for the poll loops (`wait_for_chain_lock`, `wait_for_proof`) where the next tick retries — but wrong for `resolve_status_from_info`, which is a one-shot recovery call: a transient backend error silently classified the lock as `AssetLockStatus::Broadcast` even when the underlying tx was actually chain-locked, forcing `resume_asset_lock` down the wasteful `Broadcast → wait_for_proof` path with no operator-visible signal. Split the helper: * `record_or_persister` now returns `Result<Option<TransactionRecord>, PersistenceError>`. Call sites choose their own policy. * `record_or_persister_or_log` is the new poll-loop variant — same `Option` return, but logs persister errors at `warn` (up from `debug`) before downgrading to `None`. A persistent failure is now visible in operator logs. * The two poll-loop sites (`wait_for_chain_lock`, `wait_for_proof`) switched to `_or_log`. * The two fast-fail sites (`validate_or_upgrade_proof`, `upgrade_to_chain_lock_proof`) propagate `Err` as `AssetLockProofWait("Persister lookup for tx X failed: …")`, distinct from the "neither lookup found it" message. * The one-shot recovery site (renamed `resolve_status_with_in_memory`, see #4 below) logs at `error` and degrades to `None`, preserving recovery's "always make some progress" semantics while making the failure visible. Tests updated for the new signature. Added a new test `record_or_persister_or_log_swallows_backend_errors_as_none` covering the poll-loop variant; renamed the old "swallows" test to `propagates_backend_errors` and asserts `Err`. 121/121 tests pass. ## #4 — wallet-manager lock held across persister I/O `recover_asset_lock_blocking` was acquiring `self.wallet_manager.blocking_write()` and then dispatching to `resolve_status_from_info`, which after the helper's persister fallback could perform synchronous SQLite/SwiftData/FFI I/O behind the write lock — serializing other writers on a single disk lookup. Restructured into three phases: 1. **Lock held**: claim the `tracked_asset_locks` slot, snapshot the in-memory record (only when no proof was provided), drop the lock. 2. **No lock held**: resolve status, including the persister fallback. The renamed `resolve_status_with_in_memory(snapshot, ...)` takes the pre-snapshotted record so this phase no longer touches the wallet manager. 3. **Lock held**: re-acquire to commit the `TrackedAssetLock` entry. A re-check of `tracked_asset_locks.contains_key` handles the case where another caller raced in during phase 2 (first writer wins). Net effect: the persister I/O now runs without serializing other wallet-manager writers. The lock-hold time drops from "in-memory lookup + persister round-trip" to two short in-memory operations. Verification: - `cargo fmt --all`: clean - `cargo test -p platform-wallet --lib`: 121/121 (was 120, +1 from the new `_or_log` test) - `bash packages/swift-sdk/build_ios.sh --target sim --profile dev`: BUILD SUCCEEDED end-to-end including SwiftExampleApp link Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@thepastaclaw thanks for the review. Three of the four are addressed in 372697d, the fourth was a staleness artifact: #1 — FFI not wired (stale). The review pulled commit #2 — fast-fail regression. Restored. #3 — silent error downgrade. Split the helper:
#4 — lock held across persister I/O. Restructured Tests updated: |
|
Thanks — I re-checked the current head (
I also ran |
…ery.rs The import was referenced only in a doc comment after the `resolve_status_from_info` → `resolve_status_with_in_memory` rename dropped the `&ManagedWalletInfo` parameter. Spotted by @thepastaclaw on PR #3619 review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
Follow-up to #3617. CodeRabbit's review on the rust-dashcore bump flagged a real architectural concern that I scoped out of that PR (thread): five asset-lock proof lookup sites in
wallet/asset_lock/sync/{proof,recovery}.rsresolve a transaction record from the in-memorytransactions()map keyed by(account_index, txid). With upstream'skeep-finalized-transactionsCargo feature OFF (the default), chainlocked records are evicted from that map and only their txids retained infinalized_txidsfor dedup. The chain-lock height / block hash that an asset-lock proof needs is no longer reachable through the wallet-info API.The narrow failure mode is "first lookup happens after the chainlock-eviction window."
wait_for_proofpolls and normally observesInstantSend/InBlockstate while the record is still present, but a tx that's already chain-locked at the moment the proof flow starts (e.g. crash recovery resuming an asset lock that chain-locked while we were down) hits the missing-record path.What was done?
Added a persister fallback. The persister received the full record on the chainlock-transition
storecall before eviction, so it can answer the lookup.Trait surface
New
PlatformWalletPersistence::get_core_tx_record(wallet_id, txid) -> Result<Option<TransactionRecord>, PersistenceError>in traits.rs with a defaultOk(None)impl. Backward-compatible: persisters that don't index records by txid (the in-treeNoPlatformPersistence, no-op test stubs in the wider tree) need no change. Persisters whose backing store keys records by txid (SqliteWalletPersisterin evo-tool, the SwiftData iOS persister) should override; until then those callers see the same behavior as before this PR (None response, caller's existing not-found path applies).New
WalletPersister::get_core_tx_record(txid)pub(crate)per-wallet passthrough in persister.rs.Lookup helper
record_or_persister(in_memory, persister, txid) -> Option<TransactionRecord>pub(super)helper in proof.rs. Returns the in-memory record if present, otherwise queries the persister. Persister errors are logged atdebugand surfaced asNoneso a transient backend failure doesn't turn into a hard error in the proof flow.Five sites switched to the helper
validate_or_upgrade_proofupgrade_to_chain_lock_proofwait_for_chain_lock(SPV-driven)wait_for_chain_lockwait_for_proofresolve_status_from_infofn, no persister access&self) to use the persisterHow Has This Been Tested?
5 new unit tests in
proof.rs::testscovering the helper:record_or_persister_prefers_in_memory_when_present— in-memory hit short-circuits; persister never consultedrecord_or_persister_falls_back_to_persister_on_miss— chain-lock-eviction recovery pathrecord_or_persister_returns_none_when_neither_has_it— both miss → Nonerecord_or_persister_default_persister_returns_none— confirms the trait default impl works for backends that don't overriderecord_or_persister_swallows_backend_errors_as_none— transient backend failures don't escalateFake
PlatformWalletPersistenceimplementations (FakeRecordStorekeyed by txid,ErroringStorealways-fails) added inline.cargo check --workspace --all-targetsandcargo clippy -p platform-wallet --all-targetsboth clean.Breaking Changes
None. The new trait method has a default
Ok(None)impl. The five call sites have a slightly more permissive error path (persister fallback before erroring), but the success contract is unchanged.Follow-up not in this PR
Downstream persisters (the SwiftData iOS persister, the evo-tool
SqliteWalletPersister) need to overrideget_core_tx_recordto actually benefit from this fallback. Until they do, the trait's defaultOk(None)keeps the previous behavior (caller's existing not-found path applies). Filing those overrides will close the loop on the original CodeRabbit finding for production builds.