fix(platform-wallet): lossless mpsc persistence drain — root-cause fix for the sync-watermark freeze - #4315
Conversation
|
Warning Review limit reached
Next review available in: 42 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 (2)
📝 WalkthroughWalkthroughThe wallet persistence adapter now uses a lossless unbounded channel instead of broadcast delivery. It folds events per wallet, scopes faults to rejected stores, preserves record persistence, suppresses faulted watermarks, and adds diagnostics and coverage. ChangesWallet persistence flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformWalletManager
participant WalletEventChannel
participant spawn_wallet_event_adapter
participant PersistenceStore
PlatformWalletManager->>WalletEventChannel: provide lossless WalletEvent stream
WalletEventChannel->>spawn_wallet_event_adapter: deliver buffered and new events
spawn_wallet_event_adapter->>spawn_wallet_event_adapter: fold events per wallet
spawn_wallet_event_adapter->>PersistenceStore: store wallet changesets
PersistenceStore-->>spawn_wallet_event_adapter: accept or reject changesets
spawn_wallet_event_adapter-->>PlatformWalletManager: report fault and watermark state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🔍 Review in progress — actively reviewing now (commit 733c8c5) |
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.
|
#4314 is merged ( 🤖 Posted by Claude Code |
…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>
…ying the lossless persistence channel rust-dashcore#927 (successor of #924) merged as 08bf729de819, so the temporary bfoss765-fork pin moves to the canonical dashpay rev. The pin range additionally carries #909 (out-of-order spend fix, additive) and the #927 API this consumer was built for: take_persistence_receiver (opt-in, taken once), the removed event_sender accessor (no platform callers), and the late-install warning. Cargo.lock regenerated minimally from the v4.2-dev lock: the 12 repinned git entries plus platform-wallet's log dependency edge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
777ced8 to
e864a67
Compare
|
Ready for final review/merge: rust-dashcore#927 merged upstream as 🤖 Posted by Claude Code |
The lossless persistence channel removes the broadcast-Lagged fault trigger, renaming its adapter test — the manifest's persistence.sync_fault_latch entry still cited lagged_broadcast_freezes_and_strips_subsequent_watermark. Point it at watermark_is_still_stripped_after_a_fault, which pins the latch's durable-strip semantics under the one remaining trigger (a rejected store); the sibling entry already covers the rejection latch itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… semantics + count faulted wallets once Two review follow-ups: Docs (all five layers: Rust manager, C FFI export, JNI export, Kotlin native binding, Kotlin public KDoc): the sync-fault flag's trigger and lifetime text still described the old broadcast-lag world. With the lossless persistence channel the ONE remaining trigger is a rejected persistence store(), and the latch lives for the manager INSTANCE's lifetime — a destroyed-and-recreated manager (e.g. a network switch) starts unlatched, not "process lifetime". Diagnostics: a wallet that entered a drain already faulted and had its store rejected again was counted twice in BatchDiagnostics.faulted, letting a one-wallet drain log wallets=1 faulted=2. Count each faulted wallet at most once per drain; regression test added. Addresses #4315 review findings 504063e0d935 and 30c2e8e95003. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/manager/mod.rs (1)
528-544: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the
sync_faultfield doc with this corrected contract.This doc now names a rejected
store()as the only fault trigger. The doc on thesync_faultfield ofPlatformWalletManagerstill names two triggers: astore()rejection "or a dropped-event broadcast lag". The broadcast-lag trigger no longer exists. Update that field doc so all descriptions of the latch agree.📝 Proposed field-doc correction (outside the selected range)
/// Host-visible hard sync-fault latch (dashpay/platform#4069). Set /// (and never cleared) by the wallet-event adapter the first time it - /// freezes a durable watermark after a persistence `store()` rejection - /// or a dropped-event broadcast lag. Poll via + /// freezes a durable watermark after a persistence `store()` rejection + /// — the one remaining trigger, since the lossless persistence channel + /// cannot drop or lag events. Poll via /// [`Self::sync_fault_detected`] to surface a "verification failed / /// rescan pending" state rather than re-freezing silently on the next /// launch.🤖 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/manager/mod.rs` around lines 528 - 544, Update the sync_fault field documentation on PlatformWalletManager to describe only persistence store() rejection as the latch trigger. Remove all references to dropped-event or broadcast-lag faults, while preserving the existing latch lifetime, rescan-pending, and polling behavior descriptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/sdk/sdk-parity-manifest.json`:
- Around line 857-860: Update the verification command for the manifest entry
with id watermark_is_still_stripped_after_a_fault to use that renamed test id
instead of lagged_broadcast_freezes_and_strips_subsequent_watermark. Keep the id
and command aligned, matching the convention used by the sibling manifest entry.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/mod.rs`:
- Around line 528-544: Update the sync_fault field documentation on
PlatformWalletManager to describe only persistence store() rejection as the
latch trigger. Remove all references to dropped-event or broadcast-lag faults,
while preserving the existing latch lifetime, rescan-pending, and polling
behavior descriptions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93e6095d-cb93-4f3f-85f3-7031f9976288
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomldocs/sdk/sdk-parity-manifest.jsonpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-unified-sdk-jni/src/wallet_manager.rs
The manifest entry's id was renamed but its command still filtered on the deleted lag test — cargo test exits 0 on a zero-match filter, so the persistence.sync_fault_latch gate ran zero tests and proved nothing. Point the command at watermark_is_still_stripped_after_a_fault (verified locally: 1 passed, 552 filtered out). Audited the rest of the manifest for id/command drift: none. Also aligns the last stale sync_fault doc (the manager field doc still named the removed broadcast-lag trigger). Addresses #4315 CodeRabbit finding on docs/sdk/sdk-parity-manifest.json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 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
Bug Fixes
Documentation