fix(platform-wallet): lossless mpsc persistence drain — root-cause fix for the sync-watermark freeze - #4290
fix(platform-wallet): lossless mpsc persistence drain — root-cause fix for the sync-watermark freeze#4290bfoss765 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 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.
…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 (dashpay#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 dashpay#924. dashpay#4069-safety: the channel is lossless and in-order, so every row event 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 (dashpay#4289). Requires dashpay/rust-dashcore#924 (producer) to land. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
📝 WalkthroughWalkthroughChangesWallet persistence synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WalletManager
participant PersistenceChannel
participant WalletEventAdapter
participant WalletStore
participant KotlinSDK
WalletManager->>PersistenceChannel: send WalletEvent
PersistenceChannel->>WalletEventAdapter: deliver ordered events
WalletEventAdapter->>WalletStore: persist folded batch
WalletStore-->>WalletEventAdapter: return store status
KotlinSDK->>WalletManager: query syncFaultDetected
WalletManager-->>KotlinSDK: return latched fault state
🚥 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 |
|
⛔ Blockers found — Opus deferred (commit ce9cc1a) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The lossless persistence-channel change and FFI query appear sound, but this exact head is not ready to merge because all rust-dashcore dependencies remain pinned to a contributor fork while the required upstream producer PR is still open. The new batch diagnostic also counts a rejected watermark as persisted, which makes the failure trace internally inconsistent.
Validated blockers were found in the Codex precheck. Sonnet 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 - Sonnet: not run (deferred by blocker gate)
🔴 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 `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Repin the temporary contributor-fork dependencies before merge
All eight rust-dashcore workspace dependencies now use `bfoss765/rust-dashcore` at `b5dff6de05e4a354680e5b01a54bae9e642a6ad0`. The head commit explicitly describes this as a temporary pin and states that dashpay/rust-dashcore#924 must land; that producer PR is currently open and unmerged, with this revision as its contributor-fork head. Merging this exact head would make clean production builds depend on a contributor-controlled repository rather than the project's governed upstream. After #924 lands, repin these entries to the resulting revision in `https://github.com/dashpay/rust-dashcore` and update `Cargo.lock` in the same commit.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:293-323: Count the watermark as persisted only after the store succeeds
`synced_height_persisted` is updated before `persister.store(...)` runs. When the store rejects that changeset, the batch log still reports the rejected height as persisted even though the code faults the wallet precisely because those rows and their watermark were not accepted. This contradicts the diagnostic's stated purpose of showing whether the durable watermark advanced. Capture the proposed height before moving `core`, but update the diagnostic only in the successful store arm.
| dashcore = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } | ||
| dash-network-seeds = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } | ||
| dash-spv = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } | ||
| key-wallet = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } | ||
| key-wallet-ffi = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } | ||
| key-wallet-manager = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } | ||
| dash-network = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } | ||
| dashcore-rpc = { git = "https://github.com/bfoss765/rust-dashcore", rev = "b5dff6de05e4a354680e5b01a54bae9e642a6ad0" } |
There was a problem hiding this comment.
🔴 Blocking: Repin the temporary contributor-fork dependencies before merge
All eight rust-dashcore workspace dependencies now use bfoss765/rust-dashcore at b5dff6de05e4a354680e5b01a54bae9e642a6ad0. The head commit explicitly describes this as a temporary pin and states that dashpay/rust-dashcore#924 must land; that producer PR is currently open and unmerged, with this revision as its contributor-fork head. Merging this exact head would make clean production builds depend on a contributor-controlled repository rather than the project's governed upstream. After #924 lands, repin these entries to the resulting revision in https://github.com/dashpay/rust-dashcore and update Cargo.lock in the same commit.
source: ['codex']
There was a problem hiding this comment.
Acknowledged, and the finding is correct — but it resolves upstream rather than by a change on this branch, so the pin is deliberately unchanged.
State verified just now via the API:
- fix(key-wallet-manager): lossless persistence event channel to stop the sync-watermark freeze rust-dashcore#924 — the producer that adds the lossless
mpscpersistence channel this consumer drains — is OPEN, basedev, headbfoss765:fix/wallet-event-persistence-mpsc@2af691ba,mergeStateStatus: BLOCKED,REVIEW_REQUIRED. - fix(key-wallet): out-of-order UTXO spend recorded nowhere — phantom unspent balance (#649) rust-dashcore#909 is also OPEN (
CHANGES_REQUESTED); its content is carried in the same fork revision.
There is no governed-upstream revision to point at yet: both producer PRs originate from the contributor fork into dashpay/rust-dashcore:dev, so every revision containing take_persistence_receiver() exists only on the fork today. Repinning now would mean reverting this PR's root-cause fix back onto the lossy broadcast path that caused #4069 in the first place.
So this is a genuine merge blocker and should stay flagged — the resolution is sequencing, not a code change here. Once #909 and #924 land on dashpay/rust-dashcore:dev, this branch repins all eight workspace entries to https://github.com/dashpay/rust-dashcore at the merged revision, with Cargo.lock updated in the same commit. Happy to keep this thread open as that gate.
One note for whoever performs the repin, found while verifying the above: the pinned revision b5dff6de has diverged from #924's current head (20 ahead / 1 behind). #924 has since gained f0ab7d89, which makes the persistence channel opt-in — the send half is None until take_persistence_receiver() lazily creates it. This consumer already satisfies that contract (the manager takes the receiver exactly once, before it is published to producers), so no adaptation is needed here, but the repin should target the merged dev revision rather than a rebase of b5dff6de.
| if let Some(h) = core.synced_height { | ||
| synced_height_persisted = Some(synced_height_persisted.map_or(h, |cur| cur.max(h))); | ||
| } | ||
| let cs = PlatformWalletChangeSet { | ||
| core: Some(core), | ||
| ..PlatformWalletChangeSet::default() | ||
| }; | ||
| if let Err(e) = persister.store(wallet_id, cs) { | ||
| // A rejected changeset means these rows are not on disk. Fault | ||
| // THIS wallet's watermark so it can't outrun them; the next | ||
| // scan re-emits and the idempotent upserts recover the state. | ||
| fault.fault_wallet(wallet_id, &sync_fault); | ||
| faulted_in_batch += 1; | ||
| // One-shot, unambiguous logcat marker via the `log` facade | ||
| // (android_logger forwards `log` to logcat; `tracing` may not). | ||
| if !freeze_logged { | ||
| freeze_logged = true; | ||
| log::error!( | ||
| "SYNC WATERMARK FROZEN: persister rejected a changeset for wallet {} ({}); \ | ||
| its durable sync height is now held so the next scan re-persists the \ | ||
| missing rows (dashpay/platform#4069). syncFaultDetected() is latched.", | ||
| hex::encode(wallet_id), | ||
| e | ||
| ); | ||
| } | ||
| tracing::error!( | ||
| wallet_id = %hex::encode(wallet_id), | ||
| error = %e, | ||
| "Persister rejected core changeset; freezing this wallet's sync watermark so the next scan re-persists the missing rows (dashpay/platform#4069)" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Count the watermark as persisted only after the store succeeds
synced_height_persisted is updated before persister.store(...) runs. When the store rejects that changeset, the batch log still reports the rejected height as persisted even though the code faults the wallet precisely because those rows and their watermark were not accepted. This contradicts the diagnostic's stated purpose of showing whether the durable watermark advanced. Capture the proposed height before moving core, but update the diagnostic only in the successful store arm.
| if let Some(h) = core.synced_height { | |
| synced_height_persisted = Some(synced_height_persisted.map_or(h, |cur| cur.max(h))); | |
| } | |
| let cs = PlatformWalletChangeSet { | |
| core: Some(core), | |
| ..PlatformWalletChangeSet::default() | |
| }; | |
| if let Err(e) = persister.store(wallet_id, cs) { | |
| // A rejected changeset means these rows are not on disk. Fault | |
| // THIS wallet's watermark so it can't outrun them; the next | |
| // scan re-emits and the idempotent upserts recover the state. | |
| fault.fault_wallet(wallet_id, &sync_fault); | |
| faulted_in_batch += 1; | |
| // One-shot, unambiguous logcat marker via the `log` facade | |
| // (android_logger forwards `log` to logcat; `tracing` may not). | |
| if !freeze_logged { | |
| freeze_logged = true; | |
| log::error!( | |
| "SYNC WATERMARK FROZEN: persister rejected a changeset for wallet {} ({}); \ | |
| its durable sync height is now held so the next scan re-persists the \ | |
| missing rows (dashpay/platform#4069). syncFaultDetected() is latched.", | |
| hex::encode(wallet_id), | |
| e | |
| ); | |
| } | |
| tracing::error!( | |
| wallet_id = %hex::encode(wallet_id), | |
| error = %e, | |
| "Persister rejected core changeset; freezing this wallet's sync watermark so the next scan re-persists the missing rows (dashpay/platform#4069)" | |
| ); | |
| } | |
| let synced_height = core.synced_height; | |
| let cs = PlatformWalletChangeSet { | |
| core: Some(core), | |
| ..PlatformWalletChangeSet::default() | |
| }; | |
| match persister.store(wallet_id, cs) { | |
| Ok(()) => { | |
| if let Some(h) = synced_height { | |
| synced_height_persisted = | |
| Some(synced_height_persisted.map_or(h, |cur| cur.max(h))); | |
| } | |
| } | |
| Err(e) => { | |
| // A rejected changeset means these rows are not on disk. Fault | |
| // THIS wallet's watermark so it can't outrun them; the next | |
| // scan re-emits and the idempotent upserts recover the state. | |
| fault.fault_wallet(wallet_id, &sync_fault); | |
| faulted_in_batch += 1; | |
| // One-shot, unambiguous logcat marker via the `log` facade | |
| // (android_logger forwards `log` to logcat; `tracing` may not). | |
| if !freeze_logged { | |
| freeze_logged = true; | |
| log::error!( | |
| "SYNC WATERMARK FROZEN: persister rejected a changeset for wallet {} ({}); \ | |
| its durable sync height is now held so the next scan re-persists the \ | |
| missing rows (dashpay/platform#4069). syncFaultDetected() is latched.", | |
| hex::encode(wallet_id), | |
| e | |
| ); | |
| } | |
| tracing::error!( | |
| wallet_id = %hex::encode(wallet_id), | |
| error = %e, | |
| "Persister rejected core changeset; freezing this wallet's sync watermark so the next scan re-persists the missing rows (dashpay/platform#4069)" | |
| ); | |
| } | |
| } |
source: ['codex']
There was a problem hiding this comment.
Fixed in ce9cc1a.
You were right, and it mattered more than the "suggestion" label suggests: we are reading these lines off a mainnet tester's logcat right now to decide whether the watermark is advancing, so a drain that logs synced_height_persisted=Some(h) while faulting the wallet because h's rows were not accepted points the diagnosis at the wrong subsystem.
I took your fix and went one step further, because there was a second way the line under-reported: when the fail-closed guard strips synced_height from an already-faulted wallet, the old line just showed persisted=None, which is indistinguishable from a drain that carried no watermark at all.
The commit path is now commit_batch(...) -> BatchDiagnostics, separating the three fates a height can meet within one drain:
synced_height_persisted—store()returnedOk. The only field that means the durable watermark advanced.synced_height_frozen— proposed by the batch, stripped byfreeze_synced_height_if_faultedbefore it ever reached the store.synced_height_rejected— offered to the store, which returned an error.
Each is the monotonic max across the wallets in the drain, so a batch spanning a healthy wallet and a faulted one reports both instead of collapsing into a single number.
The guard itself is untouched — this is a reporting change only. freeze_synced_height_if_faulted still runs after the fold, 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).
Two related changes in the same line:
- Dropped the hardcoded
missed=0. It reported a number the code never measured — the same defect class you flagged. Thewallet-event batch:prefix testers grep for is unchanged, and nothing in the repo parses this line. - Extracted
commit_batchout of the async loop specifically so the diagnostic is unit-testable against a realstore()rejection, rather than only reachable through the channel plumbing.
7 new tests drive the real commit_batch (production commit path, guard included): accepted / rejected / guard-stripped / watermark-only-stripped / mixed batch / monotonic max / exact line format. The regression test is mutation-verified — reintroducing the pre-fix ordering makes rejected_store_is_not_reported_as_persisted fail with left: Some(500), right: None, exactly the value you predicted.
cargo test -p platform-wallet green (538 + 9), rustfmt clean, and no new clippy findings in the touched file.
…k so the merge keeps it The log dependency was first added by the encrypted-txMetadata change (dashpay#4277), then reverted on v4.2-dev (dashpay#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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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-541: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe sync-fault docs still name a trigger this PR deletes.
core_bridge.rslines 97-99 remove the global broadcast-lag latch, because the unbounded persistence channel can never lag. A rejectedstore()is now the only condition that setssync_fault. The doc text describing both triggers was copied up through every layer and none of the copies were updated.
packages/rs-platform-wallet/src/manager/mod.rs#L528-L541: drop the "drops record-bearing events (a broadcast lag)" clause at lines 532-533, and correct line 538 — only per-wallet scoping remains inside the adapter, not "per-wallet vs. global".packages/rs-platform-wallet/src/manager/mod.rs#L394-L401: drop "or a dropped-event broadcast lag" at line 397.packages/rs-platform-wallet-ffi/src/manager.rs#L185-L190: drop "dropped record-bearing events (a broadcast lag)" at lines 185-186.packages/rs-unified-sdk-jni/src/wallet_manager.rs#L2227-L2234: drop "dropped record-bearing events" at lines 2228-2229.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt#L384-L389: drop "persistence events were dropped or" at line 386.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt#L1123-L1133: drop "dropped record-bearing events, or" at lines 1125-1126.This matters beyond wording. Integrators read these docs to decide what a
truevalue means operationally. A stale lag trigger tells them the flag can fire under normal load, when it now fires only on a genuine persistence backend error.🤖 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 - 541, Update the sync-fault documentation to state that sync_fault is triggered only when persistence store() is rejected, removing all references to dropped events or broadcast lag. In packages/rs-platform-wallet/src/manager/mod.rs lines 528-541, also correct the scoping description to say only per-wallet scoping exists inside the adapter. Apply the corresponding wording updates in packages/rs-platform-wallet/src/manager/mod.rs lines 394-401, packages/rs-platform-wallet-ffi/src/manager.rs lines 185-190, packages/rs-unified-sdk-jni/src/wallet_manager.rs lines 2227-2234, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt lines 384-389, and packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt lines 1123-1133.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/changeset/core_bridge.rs (2)
326-337: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider reducing the per-batch INFO log volume.
This line runs once per drain. During a catch-up the drain folds up to 512 events, so the volume is low. In steady state the adapter wakes per event, so this emits one logcat INFO line per wallet event. That is a noisy hot path on Android.
Two options: demote it to
log::debug!, or emit at INFO only when the batch is interesting (faulted_in_batch > 0orsynced_height_persisted.is_some()) and at debug otherwise.Separately,
missed=0is a literal in the format string. It cannot ever be nonzero, so it cannot act as the regression tripwire the comment describes. Consider dropping it or replacing it with a real counter if one becomes available.🤖 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 326 - 337, Reduce logging in the per-drain log::info! call by emitting INFO only for batches with faulted_in_batch > 0 or synced_height_persisted.is_some(), and use debug logging for ordinary batches. Remove the misleading literal missed=0 field from the message unless a real missed-event counter is available.
1369-1376: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the determinism rationale in this KDoc.
The comment states the
try_recv()drain folds in events 2..=5 "without ever awaiting". The fold loop at line 259 does awaitbuild_core_changesetfor every event. ForSyncHeightAdvancedthat future resolves without taking the manager lock, so it does not block, but the stated reason is not what makes the test deterministic.The actual guarantee is that all five events are already buffered in the channel before the task is spawned, so
try_recv()finds them regardless of whether the loop yields. State that instead. Ifbuild_core_changesetlater starts taking the read lock for more event kinds, the current wording would silently become wrong while the test still passes.🤖 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 1369 - 1376, Update the KDoc above the tokio test to remove the claim that the drain folds events without awaiting and instead explain that all five events are published before the task is spawned, ensuring try_recv() finds the buffered events regardless of loop yields. Preserve the throughput and deterministic batch-boundary explanation without asserting details about build_core_changeset’s current locking behavior.
🤖 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.
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/mod.rs`:
- Around line 528-541: Update the sync-fault documentation to state that
sync_fault is triggered only when persistence store() is rejected, removing all
references to dropped events or broadcast lag. In
packages/rs-platform-wallet/src/manager/mod.rs lines 528-541, also correct the
scoping description to say only per-wallet scoping exists inside the adapter.
Apply the corresponding wording updates in
packages/rs-platform-wallet/src/manager/mod.rs lines 394-401,
packages/rs-platform-wallet-ffi/src/manager.rs lines 185-190,
packages/rs-unified-sdk-jni/src/wallet_manager.rs lines 2227-2234,
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt
lines 384-389, and
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
lines 1123-1133.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- Around line 326-337: Reduce logging in the per-drain log::info! call by
emitting INFO only for batches with faulted_in_batch > 0 or
synced_height_persisted.is_some(), and use debug logging for ordinary batches.
Remove the misleading literal missed=0 field from the message unless a real
missed-event counter is available.
- Around line 1369-1376: Update the KDoc above the tokio test to remove the
claim that the drain folds events without awaiting and instead explain that all
five events are published before the task is spawned, ensuring try_recv() finds
the buffered events regardless of loop yields. Preserve the throughput and
deterministic batch-boundary explanation without asserting details about
build_core_changeset’s current locking behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9ea67d4-8df8-41c6-a28d-682923393b71
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlpackages/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
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Both carried-forward prior findings remain valid at head 3c57cb1: the contributor-fork dependency pin remains blocking, and the batch diagnostic still counts a rejected watermark as persisted. The latest delta only relocates the log dependency declaration and introduces no new findings; no current review reply resolves either prior thread.
Source: Codex reviewer backends gpt-5.6-sol (general) and gpt-5.6-sol (ffi-engineer); Codex verifier backend gpt-5.6-sol; the openclaw-agent coordinator is 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 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review finding on dashpay#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 (dashpay#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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: the contributor-fork dependency pin remains a blocking merge gate, while the watermark diagnostic has been fixed. New latest-delta findings: none. Cumulative reconciliation is complete.
Source: reviewer backend gpt-5.6-sol; verifier backend gpt-5.6-sol; coordinator cliproxy/gpt-5.6-sol is orchestration-only and not review evidence.
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
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
|
Moved to #4315 - in-repo branch rebased onto v4.2-dev (post-#4305). The rust-dashcore#924 dependency is repinned to the same commit rebased onto the new key-wallet rev (rebase42/lossless-persistence-channel-on-916 @ d72e71bf87 on the fork); the fork-pin clears when #924 merges. Review history preserved here. |
2 participants
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