feat(platform-wallet): expose SPV filter rescan via wallet synced-height rewind#4099
Conversation
Add a rescan-filters lever to the platform-wallet FFI + Swift SDK so the
app can re-scan compact filters from a past height (e.g. after adding
watch addresses that weren't in the filter match set when their funding
blocks were first scanned).
Mechanism — an *organic* rescan, not a direct scan. The new
`PlatformWalletManager::spv_rescan_filters_blocking` rewinds one wallet's
filter-scan checkpoint (`core_wallet.metadata.synced_height`) to
`from_height` on the shared `Arc<RwLock<WalletManager>>`. The running
`DashSpvClient` holds a clone of that exact Arc, so on the filter-sync
loop's next tick dash-spv's FiltersManager sees the wallet in
`wallets_behind(committed_height)`, calls `reset_for_rescan()`, rewinds
its committed height to the wallet's synced_height, and re-downloads /
re-matches compact filters from there.
Verified propagation chain (one shared Arc, mutation observed live):
- manager/mod.rs:115 let wallet_manager = Arc::new(RwLock::new(...))
- manager/mod.rs:154 SpvRuntime::new(Arc::clone(&wallet_manager), ...)
- manager/mod.rs:179 stored as PlatformWalletManager.wallet_manager
- spv/runtime.rs:106 DashSpvClient::new(.., Arc::clone(&self.wallet_manager), ..)
- accessors.rs (new) blocking_write() -> get_wallet_info_mut ->
core_wallet.update_synced_height(from_height)
- dash-spv sync/filters/sync_manager.rs ~205 wallet.read().wallets_behind(committed)
observes the rewound height and restarts the scan.
Semantics chosen: the WalletInterface trait's
`update_wallet_synced_height` is forward-only (silently ignores a lower
value) and so cannot rewind. This writes `core_wallet.update_synced_height`
directly — an unconditional set — which is exactly what the existing
DashPay-backfill path (`reconcile_dashpay_rescan`) uses to rewind. A
`from_height` at/above the current checkpoint is written verbatim but
arms no rescan (the loop only rescans wallets strictly behind), so a
forward/equal set is a harmless no-op rather than an error.
Persistence: the rewound checkpoint is in-memory on the WalletManager and
is not persisted by this call. That is fine for the feature — a rescan
completes in-session; if the process dies mid-rescan the wallet is simply
still behind, and the next SPV start re-arms the same backfill from the
persisted high-water. Requires SPV running for immediate effect, else it
takes effect on the next start.
Co-Authored-By: Claude Opus <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 complete (commit 0b9c98a) |
QuantumExplorer
changed the title
|
Self Reviewed |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped PR exposing an existing internal rescan mechanism through a new FFI entry point and Swift wrapper, but the core mutation has a real correctness bug: spv_rescan_filters_blocking writes from_height to synced_height unconditionally, which (per the underlying dash-spv start_download/scan_batch logic) both skips the requested block itself and can silently advance a wallet's checkpoint past unscanned blocks with no validation or error. The doc comments in both the Rust accessor and the FFI function also overstate resilience across a full process restart.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); verifier=verifier-sonnet5-4099-1783849203=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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.
In `packages/rs-platform-wallet/src/manager/accessors.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:504: Unconditional, off-by-one checkpoint write can skip the requested block or silently advance past unscanned blocks
`info.core_wallet.update_synced_height(from_height)` is an unconditional assignment with two compounding problems, both confirmed against the pinned `rust-dashcore` rev:
1. **Off-by-one**: `FiltersManager::start_download` computes `scan_start = wallet_birth_height.max(wallet_committed_height + 1)`, and the same `+1` applies when `tick()` re-arms a rescan (`progress.update_committed_height(stale_min_synced)` then `start_download`). Since `synced_height` is treated as "last completed height," writing `from_height` directly means the rescan actually starts at `from_height + 1` — the very block the caller asked to rescan is never re-scanned.
2. **No forward guard**: `scan_batch` treats any wallet whose `synced_height >= batch_end` as fully covered and skips it entirely without even testing its scripts. Because this write is unconditional, a caller passing a `from_height` above the wallet's current checkpoint (e.g. any bug in dashwallet-ios' call site) silently advances the wallet past real, unscanned blocks — with no rescan armed and no error returned. This is a silent, permanent loss of transaction history for the wallet.
Both issues stem from bypassing the wallet's normal forward-only, monotonic update path entirely.
In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:504-508: Doc comments (here and in accessors.rs) overstate recovery after a process is killed mid-rescan
Both this comment and the near-identical one in `manager/accessors.rs` (lines 488-494) claim that if the process dies mid-rescan, "the wallet is simply still behind, so the next start re-arms the same backfill from the persisted high-water." This is inaccurate. `update_synced_height` mutates the in-memory `core_wallet.metadata.synced_height` directly, bypassing the `WalletEvent::SyncHeightAdvanced` / `CoreChangeSet` path entirely. Per `changeset/changeset.rs`, the *persisted* `synced_height` is only ever updated through that event path and is monotonic-max on merge — so the on-disk high-water mark was never lowered by this call. On a full process restart, `FiltersManager::new()` reads `synced_height` from that persisted (still-high) value, which is not behind anything, so no backfill re-arms and the requested rescan is silently and permanently dropped with no signal to the caller. This differs from `reconcile_dashpay_rescan`, which is naturally retried because it recomputes its rewind target from persisted business state on every call — a one-shot rescan request has no equivalent persisted marker to retry from. Since this API targets mobile SDK consumers where process death mid-operation is common, the docs should say the rewind is in-memory-only and must be reissued after a restart, not imply automatic recovery.
| let Some(info) = wm.get_wallet_info_mut(wallet_id) else { | ||
| return false; | ||
| }; | ||
| info.core_wallet.update_synced_height(from_height); |
There was a problem hiding this comment.
🔴 Blocking: Unconditional, off-by-one checkpoint write can skip the requested block or silently advance past unscanned blocks
info.core_wallet.update_synced_height(from_height) is an unconditional assignment with two compounding problems, both confirmed against the pinned rust-dashcore rev:
- Off-by-one:
FiltersManager::start_downloadcomputesscan_start = wallet_birth_height.max(wallet_committed_height + 1), and the same+1applies whentick()re-arms a rescan (progress.update_committed_height(stale_min_synced)thenstart_download). Sincesynced_heightis treated as "last completed height," writingfrom_heightdirectly means the rescan actually starts atfrom_height + 1— the very block the caller asked to rescan is never re-scanned. - No forward guard:
scan_batchtreats any wallet whosesynced_height >= batch_endas fully covered and skips it entirely without even testing its scripts. Because this write is unconditional, a caller passing afrom_heightabove the wallet's current checkpoint (e.g. any bug in dashwallet-ios' call site) silently advances the wallet past real, unscanned blocks — with no rescan armed and no error returned. This is a silent, permanent loss of transaction history for the wallet.
Both issues stem from bypassing the wallet's normal forward-only, monotonic update path entirely.
| info.core_wallet.update_synced_height(from_height); | |
| let rewind_height = from_height.saturating_sub(1); | |
| if rewind_height < info.core_wallet.synced_height() { | |
| info.core_wallet.update_synced_height(rewind_height); | |
| } |
source: ['codex-general']
| /// Requires SPV running for an immediate effect; otherwise the rewound | ||
| /// checkpoint takes effect when SPV next starts and its filter loop | ||
| /// first ticks. The rewound checkpoint is in-memory only (not persisted | ||
| /// by this call); if the process dies mid-rescan the wallet is simply | ||
| /// still behind and the next start re-arms the backfill. |
There was a problem hiding this comment.
🟡 Suggestion: Doc comments (here and in accessors.rs) overstate recovery after a process is killed mid-rescan
Both this comment and the near-identical one in manager/accessors.rs (lines 488-494) claim that if the process dies mid-rescan, "the wallet is simply still behind, so the next start re-arms the same backfill from the persisted high-water." This is inaccurate. update_synced_height mutates the in-memory core_wallet.metadata.synced_height directly, bypassing the WalletEvent::SyncHeightAdvanced / CoreChangeSet path entirely. Per changeset/changeset.rs, the persisted synced_height is only ever updated through that event path and is monotonic-max on merge — so the on-disk high-water mark was never lowered by this call. On a full process restart, FiltersManager::new() reads synced_height from that persisted (still-high) value, which is not behind anything, so no backfill re-arms and the requested rescan is silently and permanently dropped with no signal to the caller. This differs from reconcile_dashpay_rescan, which is naturally retried because it recomputes its rewind target from persisted business state on every call — a one-shot rescan request has no equivalent persisted marker to retry from. Since this API targets mobile SDK consumers where process death mid-operation is common, the docs should say the rewind is in-memory-only and must be reissued after a restart, not imply automatic recovery.
source: ['claude-sonnet5-general', 'codex-general']
…nce executor + mnemonic error codes Three lifecycle blockers from review at 97cd577: - Use-after-free window on teardown: scope.cancel() is cooperative, so a coroutine already inside a blocking JNI call (the DashPay contact-crypto drain, a mid-call poll) kept running while close() freed the boxed PersistenceCallbacks context via nativeDestroy — a late persister.store() then dereferenced freed memory through a dropped GlobalRef. close() now joins the manager scope before nativeDestroy so every in-flight native call has returned first. - Executor leak per network switch: the handler's owned non-daemon "dash-persistence" single-thread executor was created inline as a default arg and never shut down. The handler now tracks ownership (injected dispatchers are never closed), implements AutoCloseable, and the manager closes it after nativeDestroy quiesces callbacks. - Mnemonic failures lost their code: FFIError::clean() resets code to Success, and mnemonic.rs read error.code AFTER clean() — every failure surfaced as 0/InternalError. Capture the code first (an invalid word count keeps InvalidParameter). Review suggestions in the same round: - Surface the exact DashPay send fee: the JNI now returns a 40-byte packed txid[32] || fee(u64 LE) and Dashpay.sendPayment returns SendPaymentResult(txid, feeDuffs) — the Swift (txid, feeDuffs) shape. (The prior comment claiming the fee was readable from the persisted payment entry was wrong — DashpayPaymentEntity has no fee column.) - PARITY.md: CoreContentView row downgraded to partial — the Swift-only compact-filter Rescan button + height picker (#4103 over the #4099 rescan FFI) is documented as a deferred Android gap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
There was no way for an SDK consumer to force the SPV engine to re-scan compact filters from an earlier height — needed when a wallet's filter matching may have missed transactions (e.g. after address-set changes) and the client wants a targeted re-check without a full resync. dashwallet-ios has been carrying this on its platform pin branch; upstreaming it shrinks the pin.
What was done?
platform-wallet(manager/accessors.rs): rescan entry point that rewinds the wallet's synced height so the filter loop re-fetches and re-matches filters from the requested height.platform-wallet-ffi(spv.rs):platform_wallet_manager_spv_rescan_filterswrapping it.PlatformWalletManagerSPV.swift):spvRescanFilters(fromHeight:)thin wrapper.Decision logic lives in Rust; the Swift layer only marshals, per the SDK's architectural rules.
How Has This Been Tested?
cargo check -p platform-wallet -p platform-wallet-ffi --all-targetspasses;cargo fmt --all --checkclean.build_ios.shrun on this branch: Rust slice + regenerated headers + SwiftExampleApp simulator build all succeed.Breaking Changes
None — additive API at all three layers.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code