fix(platform-wallet): batch wallet-event persistence + expose sync_fault (watermark-freeze mitigations) - #4289
Closed
bfoss765 wants to merge 2 commits into
Closed
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 dashpay#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 dashpay#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 dashpay#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 dashpay#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.
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 5, 2026 02:46
Contributor
📝 WalkthroughWalkthroughChangesThe wallet event bridge now processes bounded batches, merges changes per wallet, and latches synchronization faults. Native, JNI, and Kotlin APIs expose the latched fault state. Wallet synchronization persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Collaborator
|
✅ Final review complete — no blockers (commit e646622) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/changeset/core_bridge.rs (1)
1522-1536: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test does not guarantee that both events fold into one changeset.
After the fault latches, the adapter may already be parked in
recv(). It can then wake onblock_processed_event(wallet_id, 60)alone and store that changeset beforesync_height_event(wallet_id, 900)arrives. The assertions still pass in that interleaving, so the test is not flaky, but it does not prove the merged-watermark path the comment describes.To pin the fold, drive the merge deterministically — for example, assert the observed changeset for a batch that is fully buffered before the adapter can poll, as
buffered_events_fold_into_one_store_per_walletdoes.🤖 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 1522 - 1536, The test around the fault-latched event sends must deterministically buffer both events before the adapter can poll, then assert that the observed changeset contains the merged record and watermark. Update the test using the established synchronization pattern from buffered_events_fold_into_one_store_per_wallet, while preserving the existing fault and persistence assertions.
🤖 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 1522-1536: The test around the fault-latched event sends must
deterministically buffer both events before the adapter can poll, then assert
that the observed changeset contains the merged record and watermark. Update the
test using the established synchronization pattern from
buffered_events_fold_into_one_store_per_wallet, while preserving the existing
fault and persistence assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e87528c7-3c27-45b7-a8fc-defc34214aaf
📒 Files selected for processing (5)
packages/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/src/changeset/core_bridge.rspackages/rs-unified-sdk-jni/src/wallet_manager.rs
thepastaclaw
reviewed
Aug 5, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The wallet-event batching preserves wallet scoping and the durable-watermark guard, and the new fault-status API is consistently wired through C FFI, JNI, and Kotlin. No blocking correctness or ABI issues were confirmed. The new bounded drain loop should add coverage for its batch-limit split and mid-batch lag paths.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
🟡 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/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:281-293: Exercise the bounded-drain edge paths
The current batching tests buffer at most five events, so none crosses `ADAPTER_STORE_BATCH_LIMIT` and verifies that a burst of 513 events is split into two store rounds. The lag tests also force `RecvError::Lagged` on the initial blocking receive; none exercises `TryRecvError::Lagged` after an event has already populated `batch`. These paths enforce two distinct guarantees introduced by this loop: returning to the cancellation-aware outer `select!` after bounded work, and latching the fault before persisting a partial batch so its merged `synced_height` is stripped. Add deterministic tests for both the 512/513 boundary and a lag encountered while draining an already nonempty batch.
Comment on lines
+281
to
+293
| let mut folded = 1usize; | ||
| while folded < ADAPTER_STORE_BATCH_LIMIT { | ||
| match receiver.try_recv() { | ||
| Ok(event) => { | ||
| let wallet_id = event.wallet_id(); | ||
| let core = build_core_changeset(&wallet_manager, &event).await; | ||
| batch.entry(wallet_id).or_default().merge(core); | ||
| folded += 1; | ||
| } | ||
| Err(TryRecvError::Lagged(n)) => { | ||
| missed += n; | ||
| folded += 1; | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Exercise the bounded-drain edge paths
The current batching tests buffer at most five events, so none crosses ADAPTER_STORE_BATCH_LIMIT and verifies that a burst of 513 events is split into two store rounds. The lag tests also force RecvError::Lagged on the initial blocking receive; none exercises TryRecvError::Lagged after an event has already populated batch. These paths enforce two distinct guarantees introduced by this loop: returning to the cancellation-aware outer select! after bounded work, and latching the fault before persisting a partial batch so its merged synced_height is stripped. Add deterministic tests for both the 512/513 boundary and a lag encountered while draining an already nonempty batch.
source: ['claude', 'codex']
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes reviewable the two watermark-freeze commits that already shipped in the downstream QA build (v41int15) but were never opened as a PR. Both harden the
dashpay/platform#4069durable-watermark path.1. Batch wallet-event persistence (
3e8c27fbd0)The wallet-event adapter issued one
persister.store(..)perWalletEvent. On Android that store is a JNI hop into a Room transaction (milliseconds), while projecting an event into aCoreChangeSetis microseconds — so the drain rate was pinned at the store rate. The upstream producer publishes fire-and-forget onto a bounded broadcast ring (capacity 1000); a historical SPV catch-up outruns a consumer that slow, the ring overflows,recv()returnsLagged, and the#4069guard freezessynced_heightfor the rest of the process lifetime (a permanent latch). In the field this presented as a mainnet sync that climbs, then "rolls back" on every relaunch.Fix: block for the first event of a batch, then fold everything already buffered behind it via non-waiting
try_recv(bounded byADAPTER_STORE_BATCH_LIMIT) into onestore()per wallet. The drain rate is now bounded by the ring rather than the persister.2. Expose
sync_fault_detectedto the host (e64662205b)PlatformWalletManager::sync_fault_detected()existed since the#4069guard landed but stopped at the Rust boundary. Surface it through the full C-FFI → JNI → Kotlin path so the app can show "verification failed / rescan pending" instead of silently re-scanning from a stale height. Unconditional (outside theshieldedfeature gate), since the fault is a core-persistence signal.Note
Batching raises the burst threshold but a single
Laggedstill freezes forever. The root-cause lossless fix (dedicated unbounded persistence channel; producer side is dashpay/rust-dashcore#924) is a follow-up PR stacked on this branch.Test
cargo test -p rs-platform-walletgreen; built and shipped in v41int15.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes