fix(platform-wallet): freeze sync watermark on persistence fault — TXO loss/duplication (#4069)#4071
Conversation
… fault (dashpay#4069) The wallet-event → persister bridge in `core_bridge.rs` drains a bounded (capacity 1000) `tokio::broadcast` ring published by the upstream `WalletManager` via fire-and-forget `let _ = event_sender.send(..)`. During a historical SPV catch-up the manager processes blocks far faster than the single-threaded, JNI + Room persister can drain them, so the ring overflows and `recv()` returns `RecvError::Lagged(n)` — the dropped events are exactly the `TransactionDetected` / `BlockProcessed` records carrying new UTXOs and spent-outpoint markers. Meanwhile the separate `SyncHeightAdvanced` event (the ONLY event whose height reaches the host `syncedHeight`) keeps flowing and advances the persisted watermark past blocks whose rows never reached disk. This is the single root cause behind all three dashpay#4069 signatures: - B (store-nothing): a fresh scan floods the ring, records are dropped, yet the watermark advances → next launch rehydrates empty-and-scanned. - A (inflated balance): dropped `BlockProcessed` spends leave consumed outputs unmarked, so they rehydrate as spendable. - C (watermark outruns rows): `synced_height` is persisted in a changeset independent of the record-bearing ones, with no atomicity between them. Fix (workspace-side, in `platform-wallet`): once persistence has faulted this session — a broadcast lag OR a rejected `store()` — never advance the durable watermark again. `freeze_synced_height_if_faulted` strips only `synced_height` from every subsequent changeset (records/UTXO deltas still persist), pinning the watermark at the last fully-committed height. On the next launch the wallet restores that lower watermark and the SPV scan resumes from it, re-emitting the dropped blocks; the persister's upserts are idempotent on the outpoint key, so re-applying them restores the missing rows and re-marks the lost spends — turning the integrator's manual "clear + recreate wallet" recovery into automatic self-healing. The `Lagged` / `store()`-error logs are raised to `error!`. Requires a native rebuild (`build_android.sh`) to take effect — the change is in the Rust library, not Kotlin. Kotlin-side the watermark is already atomic with its rows *within* a changeset; added a regression test pinning that a rolled-back changeset does not advance `WalletEntity.syncedHeight`. Tests: 2 new Rust unit tests over `freeze_synced_height_if_faulted` (platform-wallet: 400 passed); 1 new Kotlin test (PlatformWalletPersistenceHandlerTest: 35 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe wallet event adapter now freezes durable sync-height advancement after persistence failures or dropped events. Tests cover watermark stripping, preservation of other changeset fields, and rollback behavior in Kotlin persistence. ChangesPersistence watermark handling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 4313d84) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: claude opus general (ok), codex gpt-5.5 general (failed), claude opus security-auditor (ok), codex gpt-5.5 security-auditor (failed), claude opus ffi-engineer (ok), codex gpt-5.5 ffi-engineer (failed). Verifier: claude opus.
Focused, well-documented fix for a persistence-vs-watermark race in the wallet event adapter. Freezing durable synced_height after any broadcast lag or store() rejection is correctly narrow — only synced_height is stripped, records/UTXOs/last_processed_height stay intact. Two non-blocking suggestions: the fault flag is task-global (freezes all wallets when one faults) and the async fault-then-freeze loop lacks integration coverage.
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:114-155: Task-global fault flag freezes every wallet's watermark on a single wallet's fault
`persistence_faulted` is a single `bool` in the shared adapter task, and every event for every wallet drains from the same broadcast subscriber. When wallet A's `store()` fails, wallet B's `synced_height` also stops advancing for the rest of the session even though wallet B's rows are still landing atomically. This is safe (worst case is a redundant rescan on next launch), which is why it's not blocking — but a `HashMap<WalletId, bool>` on the adapter would confine the freeze to the wallet that actually lost rows. A `Lagged` event has no wallet id, so that path would still have to fault every known wallet — the win is limited to the `store()`-rejection path. Worth considering if multi-wallet users are common on the JNI target where the throughput mismatch that motivated this fix is severe.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:116-178: No integration coverage for the async fault-then-freeze loop
The new Rust tests exercise `freeze_synced_height_if_faulted` in isolation and the Kotlin test pins the atomic-rollback invariant. Nothing exercises the loop itself: that a `RecvError::Lagged` actually flips `persistence_faulted`, and that a subsequent `SyncHeightAdvanced` is stripped and dropped via `is_empty_no_records`. A small test with a mock `PlatformWalletPersistence` that (a) returns `Err` on the first `store()` and asserts a following watermark-only event is not delivered, and (b) drives a real `broadcast::Sender` past its capacity to synthesize `Lagged` and asserts the same, would lock in the wiring — not just the helper. The loop is exactly where the earlier `warn!`-and-continue behavior lived and where a future refactor is most likely to silently reintroduce the bug.
| @@ -78,22 +123,34 @@ where | |||
| // state (today only `TransactionInstantLocked`, | |||
| // which checks finality before recording the IS | |||
| // lock), grab a brief read lock on the manager. | |||
| let core = build_core_changeset(&wallet_manager, &event).await; | |||
| let mut core = build_core_changeset(&wallet_manager, &event).await; | |||
| // Hold the durable watermark at the last | |||
| // fully-persisted height once persistence has | |||
| // faulted (see the guard doc above). Records/UTXOs | |||
| // in this changeset are still persisted — only the | |||
| // height advance is suppressed. | |||
| freeze_synced_height_if_faulted(&mut core, persistence_faulted); | |||
| if core.is_empty_no_records() { | |||
| // SyncHeightAdvanced for an unknown wallet, | |||
| // empty BlockProcessed, etc. — nothing to | |||
| // persist. Skip the round-trip. | |||
| // empty BlockProcessed, a watermark-only event | |||
| // stripped by the fault guard above, etc. — | |||
| // nothing to persist. Skip the round-trip. | |||
| continue; | |||
| } | |||
| let cs = PlatformWalletChangeSet { | |||
| core: Some(core), | |||
| ..PlatformWalletChangeSet::default() | |||
| }; | |||
| if let Err(e) = persister.store(wallet_id, cs) { | |||
| tracing::warn!( | |||
| // A rejected changeset means these rows are not | |||
| // on disk. Fault the watermark so it can't | |||
| // outrun them; the next scan re-emits and the | |||
| // idempotent upserts recover the state. | |||
| persistence_faulted = true; | |||
| tracing::error!( | |||
| wallet_id = %hex::encode(wallet_id), | |||
| error = %e, | |||
| "Persister rejected core changeset; state will be re-emitted on next sync round" | |||
| "Persister rejected core changeset; freezing sync watermark so the next scan re-persists the missing rows (dashpay/platform#4069)" | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: Task-global fault flag freezes every wallet's watermark on a single wallet's fault
persistence_faulted is a single bool in the shared adapter task, and every event for every wallet drains from the same broadcast subscriber. When wallet A's store() fails, wallet B's synced_height also stops advancing for the rest of the session even though wallet B's rows are still landing atomically. This is safe (worst case is a redundant rescan on next launch), which is why it's not blocking — but a HashMap<WalletId, bool> on the adapter would confine the freeze to the wallet that actually lost rows. A Lagged event has no wallet id, so that path would still have to fault every known wallet — the win is limited to the store()-rejection path. Worth considering if multi-wallet users are common on the JNI target where the throughput mismatch that motivated this fix is severe.
source: ['claude']
There was a problem hiding this comment.
✅ Fixed in 4313d843: AdapterFaultState now scopes rejected store() rounds to the affected wallet while keeping wallet-id-less broadcast lag global. fault_state_scopes_store_rejection_per_wallet_and_lag_globally covers the split.
|
On-device validation: PASSED (Galaxy S22, testnet, Android wallet parity harness from dashpay/dash-wallet#1507). With this fix in the native library: full wallet re-creation → complete chain scan → discovered TXOs persisted correctly → balance parity vs dashj confirmed → verification re-passed after app restart (the exact scenario that previously left the wallet empty-and-scanned or inflated — see #4069's three reproductions). Followed by the first successful end-to-end L1→shielded transfer through the SDK pipeline (asset lock → IS lock → Halo2 proof → Type 18 → credits minted). The integrating app's evidence gate now opens and stays open across restarts. Recommend prioritizing review — this unblocks SDK-consuming wallets generally, not just this integration. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/changeset/core_bridge.rs (1)
144-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise both fault transitions through the adapter loop.
The tests only validate the helper. Add async tests proving that a rejected store and
RecvError::Laggedactivate the guard before subsequent watermark events, while record-bearing changesets still persist.🤖 Prompt for 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. In `@packages/rs-platform-wallet/src/changeset/core_bridge.rs` around lines 144 - 168, The adapter-loop tests currently cover only the helper; add async integration tests around the adapter loop that trigger both a rejected `persister.store` and `RecvError::Lagged`, asserting each sets the persistence fault before later watermark events are processed. Also verify record-bearing changesets continue to be persisted after the guard activates, using the existing adapter loop and test persister/event symbols.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- Around line 144-168: The adapter-loop tests currently cover only the helper;
add async integration tests around the adapter loop that trigger both a rejected
`persister.store` and `RecvError::Lagged`, asserting each sets the persistence
fault before later watermark events are processed. Also verify record-bearing
changesets continue to be persisted after the guard activates, using the
existing adapter loop and test persister/event symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92b701d0-0904-4331-bd7d-698db2011c7e
📒 Files selected for processing (2)
packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/rs-platform-wallet/src/changeset/core_bridge.rs
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Codex review of c5a0358: request changes.
The watermark freeze is directionally correct, but it relies on FFIPersister.store returning Ok only after one isolated atomic callback round. That invariant is currently false. FFIPersister has no lock across begin, per-kind callbacks, and end, while core persistence, platform-address sync, shielded sync, and spawned DashPay work share it concurrently. Kotlin keeps one unsynchronized buffer per wallet and Swift keeps one global inChangeset transaction state. Interleaved rounds can therefore drop or roll back core TXO or spent-marker writes while both stores return Ok; the new fault latch remains false and a later SyncHeightAdvanced can still commit. This recreates #4069.
Before merge, serialize the entire FFI callback round with one global lock, treat a nonzero begin as fatal, reject nested begins and unmatched ends, and add a forced concurrent-store regression. Also subscribe to WalletEvents before publishing the manager: subscription currently occurs inside the spawned task, so pre-poll events are lost without Lagged. Finally, the capacity-1000 fire-and-forget channel remains lossy; the patch is fail-closed after detected lag but does not guarantee the claimed one-restart self-healing under repeatable overload. Use a backpressured or atomic persistence path, or narrow the claim and surface a hard sync fault.
The current tests do not exercise concurrent FFI rounds, actual Lagged handling, a rejected store followed by a watermark, startup timing, or two-launch recovery. This is client wallet persistence only and has no consensus impact.
| core: Some(core), | ||
| ..PlatformWalletChangeSet::default() | ||
| }; | ||
| if let Err(e) = persister.store(wallet_id, cs) { |
There was a problem hiding this comment.
P1: an Ok result here is not currently evidence that this core changeset committed atomically. FFIPersister permits overlapping begin-to-end callback rounds from core, platform-address, shielded, and DashPay writers. Kotlin can replace the open wallet buffer and Swift can commit or roll back another round, while both Rust calls return Ok. The fault flag then remains false and the later SyncHeightAdvanced is persisted past missing TXO or spend rows. Please serialize the complete FFI callback round globally and add a forced-interleaving regression.
| // corruption is unrecoverable without deleting + recreating the | ||
| // wallet. | ||
| // | ||
| // Fix: once persistence has faulted this session (a broadcast |
There was a problem hiding this comment.
This prevents a detected lag from advancing the durable cursor, but it does not guarantee the claimed self-healing on the next launch. The capacity-1000 producer remains fire-and-forget and ordinary transaction records are not restored into the in-memory map, so the same historical replay can exceed consumer throughput and lag again on every restart. Please use a lossless or backpressured persistence path, stop and rewind with throttling, or narrow this to a fail-closed mitigation and surface a hard sync fault.
| * changeset. A round that rolls back (`success = false`) must | ||
| * therefore NOT advance the persisted watermark — otherwise the | ||
| * durable watermark could outrun the rows it implies, exactly the | ||
| * "empty-and-scanned after restart" corruption in #4069. Pins the |
There was a problem hiding this comment.
This test validates the pre-existing single-threaded rollback behavior; no production Kotlin code changes in this PR, so the same test would pass on the base commit. It does not exercise RecvError::Lagged, a rejected store followed by a watermark, the stateful fault latch, or overlapping JNI store rounds. Please add adapter-level lag and store-error tests plus a concurrent FFI round test.
…er one lock (dashpay#4069) FFIPersister::store ran begin → per-kind callbacks → end with no lock, while the core-changeset bridge, platform-address sync, shielded sync, and spawned DashPay tasks all share one Arc<FFIPersister> and call store() concurrently. The host client keeps a single in-flight-round transaction state (Kotlin: one per-wallet buffer; Swift: one global inChangeset flag), so two overlapping rounds could let one round's writes land in — or roll back with — the other round's transaction, dropping core TXO / spent-marker rows while both store() calls still returned Ok. That silently bypassed the durable-watermark fault latch and recreated dashpay#4069. - Add a per-persister parking_lot::Mutex<RoundGuardState> held for the entire begin→end round, making each round atomic wrt every other. Sync (not async) mutex because store() is a synchronous trait method called blocking from both async tasks and blocking FFI entry points. - Treat a nonzero on_changeset_begin return as fatal: abort the round before any per-kind write instead of logging and continuing. - Add a defensive state machine (RoundGuardState::begin_round / end_round) that rejects a nested begin or an unmatched end as an error, never a panic, and never wedges the persister after a rejected round. - Tests: forced concurrent-store regression (probe begin/end callbacks assert no interleave), nonzero-begin abort + no-wedge, and the nested-begin / unmatched-end state-machine rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing, hard sync-fault signal (dashpay#4069) Follow-ups to the watermark-freeze guard from QuantumExplorer's review: - Subscribe-before-publish: the wallet-event broadcast receiver is now created synchronously in PlatformWalletManager::new, BEFORE the manager is wrapped in the shared Arc<RwLock> and handed to producers, then moved into the adapter task. Previously the task subscribed itself, so every event emitted between manager publication and the task's first poll was lost with no Lagged marker (a broadcast::Receiver only sees messages sent after its subscribe()). - Per-wallet fault scoping (CodeRabbit suggestion): AdapterFaultState replaces the task-global bool. A store() rejection freezes ONLY the named wallet; a broadcast Lagged (no wallet id) freezes every wallet via a single global latch that also covers wallet ids first seen later. A sibling wallet whose rows are still landing atomically keeps advancing. - Hard sync-fault signal: the adapter raises an Arc<AtomicBool> the host can poll via PlatformWalletManager::sync_fault_detected the first time it freezes a durable watermark, plus an error-level log, so integrators can surface "verification failed / rescan pending" instead of silently re-freezing on the next launch. - Narrowed the self-healing claim in the guard doc: a restart resumes from the last durable watermark, but the capacity-1000 fire-and-forget producer means repeated overload keeps freezing — this is a fail-closed safety mitigation (funds never lost/inflated), not a guaranteed one-restart self-heal. - Extracted the loop into run_wallet_event_adapter and added async integration tests exercising the loop itself: real RecvError::Lagged → fault → subsequent watermark stripped; rejected store() → watermark stripped; record-bearing changesets still persist after the guard activates; and subscribe-before-publish timing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@QuantumExplorer thanks for the review — all findings addressed. Pushed as 1. Serialize the entire FFI callback round under one lockThe P1 was correct:
One deliberate deviation from the ask: the lock is a synchronous Regression test: 2. Subscribe to WalletEvents before publishing the managerThe receiver is now created synchronously in 3. Lossy channel — narrowed claim + hard sync-fault signal
4. Per-wallet fault scoping (CodeRabbit)
5. Tests the review demanded
|
cac6927
into
dashpay:feat/kotlin-sdk-and-example-app
"Rust workspace tests / Tests (macOS)" failed at step 7 (cargo fmt --check): cac6927 landed 6 unformatted spots in persistence.rs / core_bridge.rs. Reflow only, no code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: claude opus general (ok), codex gpt-5.6-sol general (failed: revoked refresh token), claude opus security-auditor (ok), codex gpt-5.6-sol security-auditor (failed: revoked refresh token), claude opus rust-quality (ok), codex gpt-5.6-sol rust-quality (failed: revoked refresh token), claude opus ffi-engineer (ok), codex gpt-5.6-sol ffi-engineer (failed: revoked refresh token). Verifier: claude opus.
Prior-finding reconciliation (c5a03580 -> 4313d843):
- FIXED: the task-global store-fault latch is now scoped per wallet, while wallet-id-less broadcast lag remains global.
- FIXED: four async adapter-loop tests now cover real lag, rejected store, post-fault record persistence, and subscribe-before-publish delivery.
Carried-forward prior findings: none.
New findings in the latest delta: one non-blocking suggestion. A panic after begin_round() can leave the non-poisoning FFI round state latched open, causing every later store in the process to fail closed until restart. The cumulative PR otherwise closes the startup-subscription and concurrent FFI-round persistence gaps without introducing a blocking defect.
Validation: git diff --check c5a03580..4313d843; 7/7 focused platform-wallet core-bridge tests; 3/3 focused platform-wallet-ffi round-state/concurrency tests.
🟡 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-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:703-708: A panic between begin_round and end_round permanently wedges the shared FFIPersister for the process
The new round_lock is a `parking_lot::Mutex` (persistence.rs:18, 668), which — unlike `std::sync::Mutex` — does not poison on unwind. `store()` acquires it (703), calls `begin_round()` which sets `in_round=true` (708), runs the ~900-line per-kind FFI callback sequence, and only clears `in_round` via `end_round()` at line 1618. If any code in that window unwinds (an allocation failure, a `CString::new` on interior-NUL data, or a future panic in per-kind marshalling), the guard's Drop releases the mutex but leaves `in_round=true`. Every subsequent `store()` on this `Arc<FFIPersister>` — shared by the core-changeset bridge, platform-address sync, shielded sync, and DashPay tasks — then hits `begin_round()`, sees `in_round==true`, and is rejected as 'round already open' for the rest of the process; the persister never self-heals, so a single transient unwind degrades into a whole-session outage requiring a wallet restart.
Worth noting: because round_lock fully serializes rounds, only one thread is ever inside the begin→end window at a time, so `begin_round()` can realistically only observe `in_round==true` as the residue of a prior unwind — the 'concurrent nested begin' the guard nominally defends against cannot occur under the lock (a same-thread reentrant host callback would deadlock on the non-reentrant mutex before reaching `begin_round`). In other words the guard's only reachable trigger is the stale-unwind case, which it then refuses to recover from.
This is fail-closed rather than corrupting — a rejected store propagates up, the core_bridge adapter faults the wallet and freezes its watermark, so the worst case is a rescan on next launch, not lost or inflated funds — which is why it is a suggestion. It is also a genuine regression relative to the pre-PR code, which had no round lock and therefore could not wedge. The agents verified there are no `?`/`unwrap`/`expect` early returns in the window today (all failure paths set `round_success=false` and fall through to `end_round`), so this is primarily defense against a future edit or a hard panic. A small RAII drop-guard that resets `in_round` on Drop (or a `begin_round` that treats an already-open state as a recoverable stale round — logging at error! and resetting, since a prior unwind means that host transaction is already gone) would still serialize rounds and reject genuine reentrancy while letting the persister recover on the next round after a transient unwind.
| let mut round = self.round_lock.lock(); | ||
|
|
||
| // Open the round on the Rust side (rejects a nested begin / an | ||
| // unclean round left open by a prior unwind — error, never | ||
| // panic). Matched 1:1 with the `round.end_round()` below. | ||
| round.begin_round()?; |
There was a problem hiding this comment.
🟡 Suggestion: A panic between begin_round and end_round permanently wedges the shared FFIPersister for the process
The new round_lock is a parking_lot::Mutex (persistence.rs:18, 668), which — unlike std::sync::Mutex — does not poison on unwind. store() acquires it (703), calls begin_round() which sets in_round=true (708), runs the ~900-line per-kind FFI callback sequence, and only clears in_round via end_round() at line 1618. If any code in that window unwinds (an allocation failure, a CString::new on interior-NUL data, or a future panic in per-kind marshalling), the guard's Drop releases the mutex but leaves in_round=true. Every subsequent store() on this Arc<FFIPersister> — shared by the core-changeset bridge, platform-address sync, shielded sync, and DashPay tasks — then hits begin_round(), sees in_round==true, and is rejected as 'round already open' for the rest of the process; the persister never self-heals, so a single transient unwind degrades into a whole-session outage requiring a wallet restart.
Worth noting: because round_lock fully serializes rounds, only one thread is ever inside the begin→end window at a time, so begin_round() can realistically only observe in_round==true as the residue of a prior unwind — the 'concurrent nested begin' the guard nominally defends against cannot occur under the lock (a same-thread reentrant host callback would deadlock on the non-reentrant mutex before reaching begin_round). In other words the guard's only reachable trigger is the stale-unwind case, which it then refuses to recover from.
This is fail-closed rather than corrupting — a rejected store propagates up, the core_bridge adapter faults the wallet and freezes its watermark, so the worst case is a rescan on next launch, not lost or inflated funds — which is why it is a suggestion. It is also a genuine regression relative to the pre-PR code, which had no round lock and therefore could not wedge. The agents verified there are no ?/unwrap/expect early returns in the window today (all failure paths set round_success=false and fall through to end_round), so this is primarily defense against a future edit or a hard panic. A small RAII drop-guard that resets in_round on Drop (or a begin_round that treats an already-open state as a recoverable stale round — logging at error! and resetting, since a prior unwind means that host transaction is already gone) would still serialize rounds and reject genuine reentrancy while letting the persister recover on the next round after a transient unwind.
source: ['claude']
…nd-example-app Brings in DIP-13/15 DashPay invitations (#4041). Conflict resolution: - persistence.rs: kept the round_lock/RoundGuardState round serializer (a superset of v4.1-dev's store_round) + grafted #4041's persists_durably() invitation-durability attestation. Dropped v4.1-dev's duplicate store_round round test (RoundProbe collided with #4071's). - asset_lock/build.rs: kept both ensure_identity_topup_account and #4041's persist_asset_lock_account_pools; tightened broadcast_funded_asset_lock + invitation create_invitation to <ExtendedPubKeySigner> to match #4106's asset-lock signer tightening (extended the UnreachableSigner test double). - rs-unified-sdk-jni: left on_persist_invitations_fn None so Android's persists_durably() stays fail-closed (the invitation flow refuses to run without durable funding-index persistence). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Fixes the root cause behind #4069 (all three reported signatures).
Root cause
key-wallet-managerpublishes wallet persistence events on a bounded tokio broadcast ring (capacity 1000) with fire-and-forget sends. During historical SPV catch-up the producer outruns the JNI→Room consumer,recv()returnsLagged(n), and the dropped events — exactly the TXO/record/spent-marker payloads — are silently lost.SyncHeightAdvancedis the only event whose height reaches the persisted watermark (WalletEntity.syncedHeight), so the watermark advances past blocks whose rows never hit storage:Also verified NOT the cause: outpoint encoding is consistently wire-order end-to-end, and Room uses a true
@Upserton the outpoint PK.Fix (workspace-local,
rs-platform-wallet/src/changeset/core_bridge.rs)Once a persistence fault occurs in a session — broadcast
Laggedor a rejectedstore()—freeze_synced_height_if_faultedstripssynced_heightfrom every subsequent changeset (record/UTXO deltas still persist), pinning the durable watermark at the last fully-committed height. On next launch the wallet restores the lower watermark, the SPV scan resumes over the gap, and the idempotent outpoint-keyed upserts restore missing rows / re-mark lost spends — self-healing instead of silent corruption. Fault logs raised toerror!.The bounded capacity itself lives in the pinned
key-wallet-managercrate; bumping it and/or moving persistence to a backpressured channel is the recommended upstream follow-up (noted in code comments).Tests
platform-walletsuite 400 passed / 0 failed.WalletEntity.syncedHeight;:sdk:testDebugUnitTestgreen.cc @QuantumExplorer
🤖 Generated with Claude Code
Summary by CodeRabbit