fix(platform-wallet): lossless mpsc persistence drain — root-cause fix for the sync-watermark freeze - #4315
fix(platform-wallet): lossless mpsc persistence drain — root-cause fix for the sync-watermark freeze#4315bfoss765 wants to merge 5 commits into
Conversation
…rmark stops freezing The wallet-event adapter issued one `persister.store(..)` per `WalletEvent`. On Android that store is a JNI hop into a Room transaction (milliseconds), while projecting an event into a `CoreChangeSet` is microseconds - so the drain rate was pinned at the store rate, a few hundred events/sec. The upstream producer publishes fire-and-forget onto a bounded broadcast ring (`DEFAULT_WALLET_EVENT_CAPACITY`, 1000), and a historical SPV catch-up outruns a consumer that slow. The ring overflows, `recv()` returns `Lagged`, and the durable-watermark guard added for #4069 freezes `synced_height` for the rest of the process lifetime. That freeze is a *permanent* latch (`AdapterFaultState` has no clear path, by design). In the field it presented as a mainnet sync that climbs toward completion and then appears to "roll back" on every relaunch: the watermark froze shortly after install, so each restart resumed the filter scan from that same frozen height no matter how far the session had actually scanned. Fix the throughput mismatch rather than the guard: fold every event already buffered in the ring into one `CoreChangeSet` per wallet and issue a single store per batch. `CoreChangeSet` merging is commutative and associative and its `Merge` impl already anticipates exactly this fold ("a flush can fold multiple events together (TransactionDetected + BlockProcessed for the same wallet over a sync round)"), so this uses the existing contract rather than widening it. Drain rate becomes bounded by the ring instead of by the persister, which removes the overflow that trips the guard. The #4069 safety invariant is deliberately left intact - the durable watermark still must never outrun the rows it implies. Note the guard cannot simply be unfrozen on lag recovery: `keep-finalized-transactions` is off by default, so finalized `TransactionRecord`s are evicted from the in-memory wallet and the event channel is the *only* delivery path for that history. There is no source of truth to reconcile a dropped event against, so resuming watermark writes after a lag would reintroduce the silent fund-loss/inflation of #4069. Preventing the lag is the sound fix; the freeze remains as a fail-closed backstop. The freeze is now applied *after* the fold, so a `synced_height` that entered a changeset via `Merge` is stripped just like a standalone one - otherwise folding would smuggle the watermark past the guard. Tests: three new cases cover the fold (one store per wallet per batch, per-wallet scoping, and the post-fault strip of a merged watermark). All four pre-existing guard tests still pass unchanged; full crate suite 531/531.
`PlatformWalletManager::sync_fault_detected()` has existed since the #4069 watermark guard landed, but it stopped at the Rust boundary - nothing above Rust could see it. When the guard freezes a wallet's durable sync watermark, the only evidence was an error-level log line, so on Android a wallet whose watermark had frozen looked identical to one that was simply syncing slowly. Surface it through the existing four-layer path so the app can report "verification failed / rescan pending" instead of silently re-scanning from a stale height forever: - `platform_wallet_manager_sync_fault_detected` (C FFI, out-param + result code, mirroring `platform_wallet_manager_shielded_sync_is_syncing`) - `Java_..._WalletManagerNative_syncFaultDetected` (JNI) - `WalletManagerNative.syncFaultDetected` (Kotlin external) - `PlatformWalletManager.syncFaultDetected()` (Kotlin suspend wrapper) All four are unconditional - deliberately outside the `shielded` feature gate, since the fault is a core-persistence signal. Verified with and without default features so the symbol is emitted in both builds. No new FFI error codes (the standard null-pointer / invalid-handle macros are reused), so ERROR_CODE_REGISTRY.md is unchanged. The generated cbindgen header picks the symbol up automatically; no checked-in header or symbol list exists to update. Note the native flag latches for the process lifetime and never clears, so this is a one-shot poll rather than an observable flow - a UI that needs to react must check it at a lifecycle point.
…o the watermark can't freeze Root-cause follow-up to the batching + sync_fault commits on this branch. Batching raised the burst threshold but a single `broadcast::Lagged` still froze a wallet's durable sync watermark permanently (#4069). The producer (dashpay/rust-dashcore#924) now offers a dedicated, unbounded `mpsc` persistence channel alongside its lossy broadcast. This switches the consumer onto it: - core_bridge.rs: `spawn_/run_wallet_event_adapter` take `mpsc::UnboundedReceiver<WalletEvent>` instead of `broadcast::Receiver`. The batched `try_recv` fold is kept verbatim; the `Lagged`/`missed`/global `fault_all` path is removed because an unbounded channel can never lag. `AdapterFaultState` keeps only the per-wallet store-rejection freeze as a fail-closed backstop (never fires in a healthy run). - manager/mod.rs: take the receiver via `take_persistence_receiver()` instead of `subscribe_events()`. Unlike a broadcast receiver, the mpsc buffers events emitted before the task's first poll, so there is no subscribe-before-publish race. - Diagnostics via the `log` facade (android_logger forwards `log` to logcat; `tracing` may not — see rs-unified-sdk-jni JNI_OnLoad): one `log::info!("wallet-event batch: folded=.. wallets=.. synced_height_persisted=.. faulted=.. missed=0")` per drain, and a one-shot `log::error!("SYNC WATERMARK FROZEN ...")` if the per-wallet freeze ever latches — so the next tester logcat is unambiguous about whether the watermark is advancing. - Cargo.toml: re-pin key-wallet-manager (and the sibling rust-dashcore crates, kept consistent to avoid a duplicate-crate type mismatch) to the fork rev carrying #924. reaches the persister before the `SyncHeightAdvanced` watermark that implies it — the durable watermark can never outrun its rows. The freeze guard stays as a backstop but should now never fire. Tests: broadcast-driven adapter tests ported to the mpsc; the `Lagged` test is replaced by `lossless_burst_never_freezes_and_watermark_reaches_tip` (a 3000-event burst — 3× the old ring — advances the watermark to the tip with no freeze). `cargo test -p platform-wallet` (531) and `-p platform-wallet-ffi` (224) green. Stacked on the batching + sync_fault commits (#4289). Requires dashpay/rust-dashcore#924 (producer) to land. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k so the merge keeps it The log dependency was first added by the encrypted-txMetadata change (#4277), then reverted on v4.2-dev (#4279). This branch carries the log line only passively (unchanged from the merge-base), so GitHub's 3-way PR merge applies the base-side deletion and the merged Cargo.toml loses the declaration — while the log:: breadcrumb calls this branch adds in changeset/core_bridge.rs remain, producing error[E0433]: unresolved crate log in the Kotlin SDK CI build. Relocate log = "0.4" out of the reverted Logging hunk into the untouched Security region so it is a branch-owned insertion that survives the merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review finding on #4290: "the batch diagnostic still counts a rejected watermark as persisted". The per-drain batch line folded `core.synced_height` into `synced_height_persisted` BEFORE calling `persister.store(...)`, so a rejected changeset was logged as `synced_height_persisted=Some(h)` in the very drain that faulted the wallet *because* height h's rows were not accepted. We read these lines off a mainnet tester's logcat to decide whether the durable watermark is advancing, so an internally contradictory trace points the diagnosis at the wrong subsystem. This is a reporting bug, not a cosmetic nit. Split the commit path out of `run_wallet_event_adapter` into `commit_batch`, which returns a `BatchDiagnostics` distinguishing the three fates a height can meet within one drain: - `synced_height_persisted` — `store()` returned Ok. The ONLY field that means the durable watermark advanced. - `synced_height_frozen` — the fail-closed guard stripped it before it ever reached the store. Previously this collapsed to `persisted=None`, which is indistinguishable from a drain that simply carried no watermark. - `synced_height_rejected` — offered to the store, which returned an error, so the rows and the watermark are not on disk. Each is the monotonic max over the wallets in the drain, so a batch spanning a healthy wallet and a faulted one reports both rather than over-reporting one number. The fail-closed guard (#4069) is deliberately untouched — this changes REPORTING only. `freeze_synced_height_if_faulted` still strips `synced_height` after the fold, the per-wallet fault scoping is unchanged, and the one-shot `SYNC WATERMARK FROZEN` `log::error!` plus the `sync_fault` latch behave exactly as before (both asserted in the new tests). Also drops the hardcoded `missed=0` field: it reported a number the code never measured (the lossless mpsc has no drop counter), which is the same defect class as the finding above. Nothing in the repo parses this line, and the `wallet-event batch:` prefix testers grep for is unchanged. Tests: 7 new cases driving the real `commit_batch` (production commit path, guard included), covering accepted / rejected / guard-stripped / watermark-only-stripped / mixed-batch / monotonic-max / exact line format. `rejected_store_is_not_reported_as_persisted` was mutation-verified: with the pre-fix ordering reintroduced it fails with `left: Some(500), right: None`. `cargo test -p platform-wallet` green (538 + 9); rustfmt clean; clippy introduces no new findings in the touched file (the 3 pre-existing `-D warnings` errors in asset_lock/sync/recovery.rs and identity/network/withdrawal.rs are unchanged from this branch's head). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Comment |
|
⛔ Blockers found — Opus deferred (commit 777ced8) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4315 +/- ##
============================================
- Coverage 87.78% 87.61% -0.18%
============================================
Files 2677 2704 +27
Lines 342371 345211 +2840
============================================
+ Hits 300551 302446 +1895
- Misses 41820 42765 +945
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward findings: the contributor-fork pin remains a blocking merge gate, and the stale sync-fault documentation remains valid; the prior watermark-persistence diagnostic is fixed, and no current #4315 reply resolves the live items. New current-PR finding: a repeatedly rejecting, already-faulted wallet can be counted twice in one batch diagnostic.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only)..
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 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 `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Repin the temporary contributor-fork dependencies before merge
All eight rust-dashcore workspace dependencies still point to `bfoss765/rust-dashcore` at `d72e71bf870167d28438943fea92c47737ba55a5`. The PR description identifies this as a temporary pin required by dashpay/rust-dashcore#924, and the current GitHub state confirms that #924 is still open and unmerged with a contributor-fork head. The committed `Cargo.lock` also still resolves these crates from `dashpay/rust-dashcore` at `8f78baa6b7979b9bea56501ad75b5a7b7150a711`; consequently, `cargo check -p platform-wallet --locked` fails because the lockfile needs updating. After #924 lands, repin all sibling crates to the governed upstream merge revision and regenerate and commit `Cargo.lock`.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:1123-1132: Correct the sync-fault trigger and lifetime documentation
This public KDoc still says dropped record-bearing events can trigger the flag and that it remains set for the process lifetime. The new unbounded mpsc path removes the broadcast-lag trigger; `core_bridge.rs` now sets the latch only after `persister.store(...)` rejects a changeset. Each `PlatformWalletManager::new` also creates a fresh `AtomicBool(false)`, so destroying and recreating a manager resets the flag within the same process. Update this KDoc and the matching text in `WalletManagerNative.kt`, the JNI export, the C FFI export, and the Rust manager documentation to state that a store rejection freezes one wallet's watermark and latches the signal for the current manager's lifetime.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:452-453: Avoid counting an already-faulted wallet twice
When a wallet entered the drain already faulted, line 414 has already incremented `diag.faulted`. Record-bearing changesets still reach the persister after their watermark is stripped, so another rejection reaches this error arm and increments the same wallet again. A one-wallet drain can therefore log `wallets=1 ... faulted=2`, contradicting the diagnostic field's wallet-count wording. Increment here only when this rejection newly faults the wallet.
| dashcore = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } | ||
| dash-network-seeds = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } | ||
| dash-spv = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } | ||
| key-wallet = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } | ||
| key-wallet-ffi = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } | ||
| key-wallet-manager = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } | ||
| dash-network = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } | ||
| dashcore-rpc = { git = "https://github.com/bfoss765/rust-dashcore", rev = "d72e71bf870167d28438943fea92c47737ba55a5" } |
There was a problem hiding this comment.
🔴 Blocking: Repin the temporary contributor-fork dependencies before merge
All eight rust-dashcore workspace dependencies still point to bfoss765/rust-dashcore at d72e71bf870167d28438943fea92c47737ba55a5. The PR description identifies this as a temporary pin required by dashpay/rust-dashcore#924, and the current GitHub state confirms that #924 is still open and unmerged with a contributor-fork head. The committed Cargo.lock also still resolves these crates from dashpay/rust-dashcore at 8f78baa6b7979b9bea56501ad75b5a7b7150a711; consequently, cargo check -p platform-wallet --locked fails because the lockfile needs updating. After #924 lands, repin all sibling crates to the governed upstream merge revision and regenerate and commit Cargo.lock.
source: ['codex']
| /** | ||
| * Whether the native manager has frozen its durable sync watermark this | ||
| * session (dashpay/platform#4069). `true` means the wallet-event adapter | ||
| * dropped record-bearing events, or a persistence `store()` was rejected, | ||
| * so the persisted `syncedHeight` is deliberately held behind the chain | ||
| * tip and a rescan is pending on the next launch. Poll this to surface a | ||
| * hard "verification failed / rescan pending" state instead of leaving | ||
| * the fault visible only in the error logs. | ||
| * | ||
| * The flag latches: once `true` it stays `true` for the process lifetime. |
There was a problem hiding this comment.
🟡 Suggestion: Correct the sync-fault trigger and lifetime documentation
This public KDoc still says dropped record-bearing events can trigger the flag and that it remains set for the process lifetime. The new unbounded mpsc path removes the broadcast-lag trigger; core_bridge.rs now sets the latch only after persister.store(...) rejects a changeset. Each PlatformWalletManager::new also creates a fresh AtomicBool(false), so destroying and recreating a manager resets the flag within the same process. Update this KDoc and the matching text in WalletManagerNative.kt, the JNI export, the C FFI export, and the Rust manager documentation to state that a store rejection freezes one wallet's watermark and latches the signal for the current manager's lifetime.
source: ['codex']
| fault.fault_wallet(wallet_id, sync_fault); | ||
| diag.faulted += 1; |
There was a problem hiding this comment.
🟡 Suggestion: Avoid counting an already-faulted wallet twice
When a wallet entered the drain already faulted, line 414 has already incremented diag.faulted. Record-bearing changesets still reach the persister after their watermark is stripped, so another rejection reaches this error arm and increments the same wallet again. A one-wallet drain can therefore log wallets=1 ... faulted=2, contradicting the diagnostic field's wallet-count wording. Increment here only when this rejection newly faults the wallet.
| fault.fault_wallet(wallet_id, sync_fault); | |
| diag.faulted += 1; | |
| fault.fault_wallet(wallet_id, sync_fault); | |
| if !is_faulted { | |
| diag.faulted += 1; | |
| } |
source: ['codex']
2 participants
Continues #4290 — moved from a fork branch to an in-repo branch (rebased onto v4.2-dev post-#4305) so maintainers can push changes directly, per review request. Full review history on #4290.
Summary
Root-cause fix for the mainnet sync-watermark freeze (#4069). Stacked on #4289 (batching +
sync_faultexposure) — the first two commits here are #4289; review the top commit. Requires the producer PR dashpay/rust-dashcore#924 to land.Batching (#4289) raised the burst threshold but a single
broadcast::Laggedstill froze a wallet's durable sync watermark permanently. The producer (#924) now offers a dedicated, unboundedmpscpersistence channel alongside its lossy broadcast; this switches the consumer onto it.Changes (top commit)
spawn_/run_wallet_event_adaptertakempsc::UnboundedReceiver<WalletEvent>instead ofbroadcast::Receiver. The batchedtry_recvfold is kept verbatim. TheLagged/missed/ globalfault_allpath is removed — an unbounded channel can never lag.AdapterFaultStatekeeps only the per-wallet store-rejection freeze as a fail-closed backstop.take_persistence_receiver()instead ofsubscribe_events(). The mpsc buffers events emitted before the task's first poll, so there is no subscribe-before-publish race.logfacade (android_logger forwardslogto logcat at Info;tracingmay not): onelog::info!("wallet-event batch: folded=.. wallets=.. synced_height_persisted=.. faulted=.. missed=0")per drain, and a one-shotlog::error!("SYNC WATERMARK FROZEN …")if the freeze ever latches — so the next tester logcat is unambiguous.key-wallet-manager(and the sibling rust-dashcore crates, kept consistent to avoid a duplicate-crate type mismatch) to the fork rev carrying feat: persist ephemeral state #924. (The shipping v41int16 AAR builds against a rev of feat: persist ephemeral state #924 rebased onto the integration branch's rust-dashcore base; this branch pins the v4.2-dev-based rev for a minimal, compilable review.)#4069-safety
The channel is lossless and in-order, so every
TransactionDetected/BlockProcessedrow event reaches the persister before theSyncHeightAdvancedwatermark that implies it — the durable watermark can never outrun its rows. The freeze guard stays as a fail-closed backstop but should now never fire.Why unbounded, not bounded back-pressure
Several producer emit sites run inside the manager's
RwLockwrite guard, while this consumer needs aread()lock on the same manager to project each event. A boundedsend().await/blocking_sendparked under the write guard would deadlock this consumer. Unbounded keeps the producer lock-safe while still lossless. See #924 for the full argument.Test
Broadcast-driven adapter tests ported to the mpsc; the
Laggedtest is replaced bylossless_burst_never_freezes_and_watermark_reaches_tip(a 3000-event burst — 3× the old ring — advances the watermark to the tip with no freeze).cargo test -p platform-wallet(531) and-p platform-wallet-ffi(224) green;cargo fmt --checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes