fix(key-wallet-manager): lossless persistence event channel to stop the sync-watermark freeze - #924
Closed
bfoss765 wants to merge 14 commits into
Closed
fix(key-wallet-manager): lossless persistence event channel to stop the sync-watermark freeze#924bfoss765 wants to merge 14 commits into
bfoss765 wants to merge 14 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds an optional unbounded persistence event channel. Wallet events now use centralized dual-channel emission. Processing paths retain event order. A regression test verifies lossless persistence delivery during a 5,000-event burst. ChangesPersistence event delivery
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProcessBlock
participant WalletManager
participant PersistenceReceiver
participant BroadcastReceiver
ProcessBlock->>WalletManager: emit_event(WalletEvent)
WalletManager->>PersistenceReceiver: send cloned event
WalletManager->>BroadcastReceiver: broadcast original event
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@key-wallet-manager/src/lib.rs`:
- Around line 201-210: Update WalletManager::emit_event to handle a closed
persistence_sender instead of ignoring its SendError. Propagate the delivery
failure through the existing thiserror error type, or transition to a terminal
sync-failure state that prevents subsequent in-memory state advancement;
preserve normal fan-out behavior for event_sender.
- Around line 153-173: Update WalletManager::new, persistence_receiver
initialization, and take_persistence_receiver so an unbounded persistence queue
is not created or retained unless a draining consumer is installed and
supervised before event emission. Ensure emit_event cannot enqueue indefinitely
when no consumer exists, while preserving the existing persistence delivery
behavior for configured consumers and updating affected construction paths
accordingly.
🪄 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: 78213015-5688-4316-a31e-198e7ad6fd44
📒 Files selected for processing (4)
key-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #924 +/- ##
==========================================
- Coverage 75.00% 74.99% -0.01%
==========================================
Files 328 328
Lines 77228 77253 +25
==========================================
+ Hits 57925 57938 +13
- Misses 19303 19315 +12
|
bfoss765
force-pushed
the
fix/wallet-event-persistence-mpsc
branch
from
August 5, 2026 02:56
baf975f to
b5dff6d
Compare
…efore-funding (dashpay#649) A spend processed BEFORE the transaction that funded the UTXO it spends (out-of-order block delivery during a cold rescan) leaves that UTXO permanently in the wallet's tracked set, producing phantom spendable balance. Device evidence (testnet): output 2febe5d7e8ad1dd0fb633004a82a24783d9b2e9095883541576e5f1344eb9975:0 (1,000,000 duffs) is counted unspent by the SDK while dashj has it spent, and the phantom +0.01 survives a full wallet rebuild from seed (fresh re-derivation + rescan reproduces it deterministically), proving the miss lives in the scan/processing path, not just live mempool ingestion. This test models that scenario at the WalletManager level: fund 1,000,000 duffs to a wallet address, then deliver the spending block (height 200) BEFORE the funding block (height 100). It asserts the funding outpoint is NOT still tracked afterward. FAILS on the current pin (which already contains dashpay#837/dashpay#864/dashpay#891/dashpay#893) — those do not address this defect. Refs: dashpay#649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ashpay#649 Root cause: the "already spent" guard in managed_core_funds_account.rs::update_utxos keys off the ACCOUNT-LOCAL `spent_outpoints` set, which is only populated when the account itself processes the spending transaction. When a spend is delivered before its funding tx (out-of-order rescan), the wallet does not yet own the input, so the spend is classified as irrelevant, update_utxos never runs for it, and nothing records the spend. When the funding tx is processed later, the output is (re-)inserted as a fresh, spendable UTXO -> phantom balance that survives a full from-seed rescan. Fix (adapted from dashpay#851): record every spend observed in a block into a new wallet-level `observed_spent_outpoints` map (ManagedWalletInfo), independent of the spending tx's classification or account attribution. update_utxos and record_transaction consult this map: - update_utxos skips any output already observed spent (spend-first ordering: funding arrives after the spend). - remove_spent_from_accounts drops a coin the matched-account path missed (funding-first ordering: spend routed to another account). - TransactionRecord::compensate_for_observed_spends keeps net_amount / output_details consistent with the observed spend (declarative, so it is idempotent across rescan replays). The set is bounded-permanent: entries are evicted by prune_finalized_observed_spends once the spend height is provably final (<= min(chainlock height, synced_height)); add-account rewinds the sync checkpoint so a late account gets filter coverage before pruning can run. A dash-spv commit-time contiguity guard keeps a mid-flight account-add rescan from being silently clobbered forward. Also fixes AddressPool::prune_unused to clear script_pubkey_index alongside address_index. Adds manager-level regression tests (multi-wallet, large-block stress) that exercise the fix through the public WalletManager API. The repro test from the previous commit now passes; key-wallet (549), key-wallet-manager (all) and dash-spv lib (482) suites are green. The pre-existing masternode-network integration failures (test_utils/masternode_network.rs:106) are unrelated and fail identically on the clean pin. Refs: dashpay#649 Adapted-from: dashpay#851 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…patch coverage Codecov flagged the dashpay#649 fix's previously-uncovered branches: the wallet-level `observed_spent_outpoints` serde adapter, finality-boundary pruning, the funding-first removal guard, the account-add sync rewind, and the AddressPool `script_pubkey_index` prune fix. The manager-level integration tests only drive the spend-first ordering end-to-end, leaving these reachable only from the crate-internal `pub(crate)` surface. Add `key-wallet/src/tests/observed_spent_outpoints_tests.rs` (the sibling file already referenced by observed_spent_large_block_stress_test.rs) with five white-box tests, plus one AddressPool prune test: - observed_spent_outpoints_survive_serde_round_trip: exercises the (OutPoint, height) sequence serde adapter (serialize + deserialize visitor) and the empty-map / `#[serde(default)]` path, isolated on an account-less wallet so the populated-account `script_pubkey_index` JSON-key blocker does not apply. - prune_finalized_observed_spends_respects_finality_boundary: no-op without a chainlock; otherwise evicts exactly entries at/below min(chainlock height, synced_height), keeping the rescan case (chainlock above sync checkpoint) from over-pruning. - funding_first_guard_removes_held_coin_and_compensates_record: the un-gated remove_spent_from_accounts / finalize_guard_removed_utxo path — coin dropped, reservation released, funding record compensated to net 0; idempotent; coinbase skipped. - wallet_level_set_outlives_account_local_reload: the account-local spent_outpoints derived set (rebuilt from recorded txs via simulate_reload_rebuild_spent_outpoints) forgets an unrecorded spend, but the persisted wallet-level set still prevents resurrection on funding re-delivery. - adding_account_from_xpub_rewinds_sync_checkpoint: standalone-account add collapses synced_height to birth_height - 1; a still-behind checkpoint is left untouched. - prune_unused_clears_script_pubkey_index (address_pool_tests.rs): regression guard for the missing script_pubkey_index.remove in AddressPool::prune_unused. key-wallet lib (554) and key-wallet-manager (all) suites green. Refs: dashpay#649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… modification, tighten deser cap - compensate_for_observed_spends: only replace the match-derived net_amount when the compensation actually dropped an output detail, keeping the no-observed-spend path byte-identical to pre-dashpay#649 behavior; pinned by a new unit test. - record_observed_spends: report whether the persisted observed-spent map actually changed, and surface that as state_modified in check_core_transaction — a consumer persisting only on reported modifications must not lose a recorded spend across a restart; pinned by a new regression test (new spend reports, unchanged redelivery and mempool spends do not). - Replace a comment reference to a nonexistent test with the inline rationale for why input_details and account_match.sent populate together. - Tighten MAX_OBSERVED_SPENT_OUTPOINTS 10M -> 1M (load-time allocation cap from a few hundred MB to a few tens of MB), still far above any legitimate size. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…urface guard-rewritten records, restore public account API Addresses three external review findings on top of PR dashpay#909: 1. [P1] The commit-time contiguity guard could still certify unscanned coverage for a newly added account: a rewind landing INSIDE a scanned batch's range passed the height check, and an account add that moved no heights (checkpoint already at the birth floor) was undetectable by any height comparison. ManagedWalletInfo now carries an in-memory account_generation counter bumped on every account add (even height-invisible ones); filter scan snapshots it per wallet and commit refuses to advance a wallet whose generation changed since scan. Pinned by three new dash-spv tests including the mid-batch rewind repro (9000 -> scan [5000..9999] -> rewind 7499 -> commit keeps 7499) and the unmoved-checkpoint case. 2. [P2] The funding-first guard rewrote funding records without ever surfacing them: remove_spent_from_accounts now returns post-compensation clones of every rewritten record, the checker adds them to updated_records on both the relevant and irrelevant paths, and the manager propagates updated_records independent of is_relevant, so consumers persisting per-record updates see the rewrite. 3. [P2] ManagedAccountRefMut::record_transaction/confirm_transaction had silently gone pub -> pub(crate) with changed signatures. The public methods are restored with their original signatures (recording with no observed-spend context, the pre-dashpay#649 behavior); the checker uses new pub(crate) *_with_observed_spends variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… history (dashpay#649/dashpay#846) HashEngineering reported (against dashpay#851/dashpay#866) that a funding transaction recovered after its spend was already observed -- every wallet-relevant output already in observed_spent_outpoints before the funding is applied -- was dropped from history entirely: the spend-first path made it come out not-relevant, so no TransactionRecord and no detection event were produced, while balance and UTXO set stayed exact. That record-loss "belongs in dashpay#851"; dashpay#851 is superseded by dashpay#909. Investigation of dashpay#909 shows its core commit (ebcd40a) already implements the suggested remedy, so no behavior change is needed: - relevance in check_transaction_for_match is address-membership based and is never gated on spent-status, so a fully-spent funding tx is still classified relevant; - ManagedCoreFundsAccount::record_transaction unconditionally inserts the record after TransactionRecord::compensate_for_observed_spends zeroes the already-spent outputs (net 0, no UTXO); - the "never insert already-spent value" guard lives in update_utxos (UTXO insertion only), not in recording. The QuantumExplorer "surface updated records independent of relevance" review fix covers the separate funding-first UPDATE case (remove_spent_from_accounts rewriting an existing funding record); the born-fully-spent NEW-record insertion is covered independently by the record_transaction compensate path. The existing observed-spent tests assert only UTXO/balance, leaving the history-record guarantee uncovered. This adds that coverage: two tests pin that a born-fully-spent recovered tx -- a plain funding tx, and a CoinJoin- style intermediate hop that spends a live coin -- is surfaced as a new record and recorded in the account's transaction history, while balance and UTXOs stay at zero. Verified across InBlock and InChainLockedBlock (chainlocked recovery) contexts and the WalletManager block path during investigation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… job passes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…elper CodeRabbit: the shared `spend_tx` helper hardcoded `Address::dummy(Network::Testnet, ..)`, forcing every observed-spend test onto Testnet (coding guideline: never hardcode network parameters in a shared helper). Add a `network: Network` parameter and thread it into `Address::dummy`; each caller passes the network its manager uses. All key-wallet-manager tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@key-wallet-manager/src/lib.rs`:
- Around line 153-174: Replace the unbounded persistence channel represented by
WalletManager::persistence_sender with a bounded handoff, and refactor
emit_event and its callers so persistence projection occurs outside the manager
write-lock lifetime before applying backpressure. Define explicit behavior when
the bounded queue is full, such as awaiting delivery or using the established
durable recovery path, while preserving take_persistence_receiver’s opt-in
contract and handling persistence_consumer_lost without allowing unbounded event
growth.
🪄 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: 4d2dc67e-7f18-4c97-9b6e-59c650c3434a
📒 Files selected for processing (4)
key-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- key-wallet-manager/src/event_tests.rs
- key-wallet-manager/src/process_block.rs
…d-spend mechanism Applies ZocoLini's requested simplification (PR dashpay#909 review): the dashpay#649 out-of-order-spend fix is the two-piece mechanism only — (a) record every input seen in a block-context tx into `observed_spent_outpoints`, independent of classification (`wallet_checker.rs`), and (b) in `update_utxos`, skip inserting an output whose outpoint is already in that map (`managed_core_funds_account.rs`). Removes the separate unattributable-spend compensation machinery that was bundled in, which also made transaction history order-dependent (a receive delivered before its spend was rewritten to a 0-value entry, erasing it from history — a spend delivered first leaves it intact): - `TransactionRecord::compensate_for_observed_spends` and its unit tests - `ManagedWalletInfo::remove_spent_from_accounts` (both call sites in `check_core_transaction`) and its `finalize_guard_removed_utxo` helper - the now-dead account-local helpers `mark_outpoint_spent`, `release_reservation_for`, and the test-only `simulate_reload_rebuild_spent_outpoints` - the `updated_records`-independent-of-relevance change in `WalletManager` (its only source was the removed funding-first guard) The `record_transaction_with_observed_spends` / `confirm_transaction_with_observed_spends` pair is kept: it is the plumbing that delivers the wallet-level observed map to `update_utxos`, i.e. piece (b) itself. A born-fully-spent funding tx is still recorded in history; its already-spent output is simply never (re-)tracked as a UTXO (balance/UTXO correctness comes from piece (b), not from rewriting the record). Tests updated to pin that the receive is preserved in history rather than erased. Test-helper cleanup (`key-wallet-manager/tests/common/mod.rs`): drop the superfluous `Network` parameter from `spend_tx` (every caller passed Testnet and the payee's network is irrelevant to observed-spend logic) and build the external payee script directly, so the helper needs no network at all. All three observed-spend integration tests (incl. the deterministic repro) and `cargo test -p key-wallet --lib` pass; workspace clippy (`-D warnings`, debug + release) and rustfmt are clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
force-pushed
the
fix/wallet-event-persistence-mpsc
branch
from
August 5, 2026 15:20
d7fe2ec to
58be4d7
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
key-wallet/src/tests/mod.rs (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider placing the new tests in
spent_outpoints_tests.rs.The path instructions enumerate the unit-test module files for
key-wallet/src/tests/, andobserved_spent_outpoints_tests.rsis not in that list.spent_outpoints_tests.rsalready exists as the listed module for spent-outpoint behavior.If the observed-spend cases are intentionally kept separate from the existing spent-outpoint cases, keep the new module and update the organization list instead. Otherwise, move the tests into
spent_outpoints_tests.rs.As per path instructions: "Organize unit tests by functionality into separate test modules: account_tests.rs, address_pool_tests.rs, transaction_tests.rs, wallet_tests.rs, integration_tests.rs, balance_tests.rs, spent_outpoints_tests.rs, ...".
🤖 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 `@key-wallet/src/tests/mod.rs` at line 25, Organize the observed-spend tests with the existing spent-outpoint tests by moving the contents of observed_spent_outpoints_tests.rs into spent_outpoints_tests.rs and removing its module declaration from the tests module. If separation is required, retain the module and update the test organization list to include it.Source: Path instructions
🤖 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 `@key-wallet-manager/tests/out_of_order_spend_repro_test.rs`:
- Line 40: Rename the test function
spend_processed_before_its_funding_tx_leaves_utxo_permanently_tracked to reflect
the asserted invariant that the UTXO is not still tracked after the flow
completes. Update only the test name in out_of_order_spend_repro_test.rs,
keeping the existing spend_processed_before_its_funding_tx scenario and the
!still_tracked assertion unchanged so the test name matches the expected
behavior.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 63-83: The observed-spend insertion in the wallet-checking flow
must be persisted independently of transaction relevance. Update the FFI
result/return path to expose and honor result.state_modified, including when
is_relevant is false, or make observed_spent_outpoints durable immediately
within record_observed_spends; ensure unrelated blocked spends survive restart.
---
Nitpick comments:
In `@key-wallet/src/tests/mod.rs`:
- Line 25: Organize the observed-spend tests with the existing spent-outpoint
tests by moving the contents of observed_spent_outpoints_tests.rs into
spent_outpoints_tests.rs and removing its module declaration from the tests
module. If separation is required, retain the module and update the test
organization list to include it.
🪄 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: bc2fdd65-f467-42ec-8e6a-1f38978342fa
📒 Files selected for processing (22)
dash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/manager.rskey-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet-manager/tests/common/mod.rskey-wallet-manager/tests/observed_spent_large_block_stress_test.rskey-wallet-manager/tests/observed_spent_multi_wallet_test.rskey-wallet-manager/tests/out_of_order_spend_repro_test.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_ref.rskey-wallet/src/managed_account/managed_core_funds_account.rskey-wallet/src/tests/address_pool_tests.rskey-wallet/src/tests/mod.rskey-wallet/src/tests/observed_spent_outpoints_tests.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/managed_accounts.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- key-wallet-manager/src/event_tests.rs
- key-wallet-manager/src/accessors.rs
- key-wallet-manager/src/lib.rs
…iant it pins The test asserted `!still_tracked` — the funding UTXO must NOT remain in the tracked set once its spend was observed first — but was named `..._leaves_utxo_permanently_tracked`, i.e. after the dashpay#649 bug rather than after the pinned behaviour. A failure therefore read as the expected outcome. Rename to `..._does_not_leave_utxo_tracked`; assertions and scenario are unchanged.
…he sync-watermark freeze
The wallet event bus is a single bounded `tokio::broadcast` ring
(`DEFAULT_WALLET_EVENT_CAPACITY` = 1000), fanned out fire-and-forget from
`process_block.rs`. It feeds two very different consumers:
1. incidental subscribers reached via `subscribe_events()` — dash-spv's
`EventHandler` dispatch and the unit tests — for which a dropped event
under overload is harmless; and
2. the platform durable-persistence consumer, for which a dropped event is
NOT harmless: the dropped `TransactionDetected`/`BlockProcessed` rows
never reach disk while the surviving `SyncHeightAdvanced` watermark keeps
advancing, so the persisted sync height outruns the rows it implies. The
platform `#4069` guard then latches that wallet's watermark into a
*permanent* freeze for the rest of the process. In the field this is the
mainnet sync that climbs, then "rolls back" to a stale height on every
relaunch.
Under a heavy historical SPV catch-up a burst larger than the ring between
consumer drains overflows it (`RecvError::Lagged`) and trips exactly that
freeze. Batching the persistence stores (shipped earlier) raised the burst
threshold but a single lag still freezes forever.
Fix: give the persistence consumer its own dedicated, unbounded
`tokio::mpsc` channel that carries the same event stream, in the same order,
with no drops — so it can never observe `Lagged` and its freeze guard never
fires. The existing broadcast is kept unchanged for the incidental
subscribers (no behavioural change for dash-spv or tests).
Why unbounded rather than a bounded back-pressuring channel: several emit
sites run inside this manager's `RwLock` write guard (SPV block processing
holds `wallet.write().await` across `process_block_for_wallets`), while the
persistence consumer needs a `read()` lock on the *same* manager to project
each event. A bounded `send().await`/`blocking_send` that parked under the
write guard would deadlock the very consumer that must drain it. A
non-blocking unbounded enqueue keeps every emit path lock-safe while still
guaranteeing losslessness. All emission now flows through a single
`emit_event` choke point that pushes to both channels in order.
`#4069`-safety: unchanged ordering + zero drops means the persistence
consumer sees every row event before the watermark event that implies it,
exactly as the non-lagged broadcast path already guaranteed; the durable
watermark can never outrun its rows.
- lib.rs: add `persistence_sender`/`persistence_receiver` + `emit_event`.
- accessors.rs: add `take_persistence_receiver()` (handed to the consumer once).
- process_block.rs: route all six emit sites through `emit_event`.
- event_tests.rs: prove a 5000-event burst is delivered losslessly and in
order on the persistence channel while the bounded broadcast lags.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ace a lost consumer Addresses two CodeRabbit findings on the lossless persistence mpsc: - Stability & Availability: `WalletManager::new` unconditionally created and retained `mpsc::unbounded_channel()`, so a manager whose consumer is never installed (only tests call `take_persistence_receiver`) would accumulate every emitted event without bound. Make persistence delivery opt-in: the channel is now created lazily by `take_persistence_receiver` (the send half is `None` until a consumer installs itself), so `emit_event` enqueues nothing when no consumer exists. Unbounded + non-blocking is retained for installed consumers, which is required: several emit sites run inside the manager's RwLock write guard while the consumer needs a read lock to drain, so a bounded blocking send would deadlock, and a bounded try_send that dropped would reintroduce the watermark freeze (#4069). The documented take-before-emit contract means no pre-consumer events are lost. - Data Integrity: `emit_event` ignored `SendError`. If the installed consumer drops its receiver while the manager is still running, durable persistence has silently stopped. Surface it: log once (latched) that the consumer was lost. We deliberately do not halt in-memory advancement — the manager does not own durable state, and on restart the wallet re-scans from the last persisted height, so a lost consumer causes no durable corruption. All key-wallet-manager tests pass, including the lossless-burst persistence test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…_event Collapse the hand-wrapped `&&` condition onto one line to satisfy cargo fmt --check (the pre-commit gate). Formatting only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
force-pushed
the
fix/wallet-event-persistence-mpsc
branch
from
August 5, 2026 17:03
58be4d7 to
2af691b
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Member
|
Reviewed in depth (verification details in the successor's description). With #909 merged, the rebase + the one review finding are now in #927 — same three commits with authorship preserved, moved to an in-repo branch, plus the late-install warning ( 🤖 Posted by Claude Code |
Contributor
Author
QuantumExplorer
pushed a commit
to dashpay/platform
that referenced
this pull request
Aug 6, 2026
…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>
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
The wallet event bus is a single bounded
tokio::broadcastring (DEFAULT_WALLET_EVENT_CAPACITY= 1000), fanned out fire-and-forget fromprocess_block.rs. It feeds two very different consumers:subscribe_events()— dash-spv'sEventHandlerdispatch and the unit tests — for which a dropped event under overload is harmless.dashpay/platformrs-platform-wallet), for which a dropped event is not harmless: droppedTransactionDetected/BlockProcessedrows never reach disk while the survivingSyncHeightAdvancedwatermark keeps advancing, so the persisted sync height outruns the rows it implies. The platform#4069guard then latches that wallet's watermark into a permanent freeze. In the field this presents as a mainnet sync that climbs, then "rolls back" to a stale height on every relaunch.Under a heavy historical SPV catch-up a burst larger than the ring between consumer drains overflows it (
RecvError::Lagged) and trips exactly that freeze. Batching the persistence stores (shipped separately on the platform side) raised the burst threshold, but a single lag still freezes forever.Fix
Give the persistence consumer its own dedicated, unbounded
tokio::mpscchannel carrying the same event stream, in the same order, with no drops — so it can never observeLaggedand its freeze guard never fires. The existing broadcast is kept unchanged for the incidental subscribers (no behavioural change for dash-spv or tests). All emission now flows through a singleemit_eventchoke point that pushes to both channels in order.Why unbounded rather than a bounded back-pressuring channel
Several emit sites run inside this manager's
RwLockwrite guard (SPV block processing holdswallet.write().awaitacrossprocess_block_for_wallets), while the persistence consumer needs aread()lock on the same manager to project each event. A boundedsend().await/blocking_sendthat parked under the write guard would deadlock the very consumer that must drain it. A non-blocking unbounded enqueue keeps every emit path lock-safe while still guaranteeing losslessness.#4069-safetyUnchanged ordering + zero drops means the persistence consumer sees every row event before the watermark event that implies it — exactly as the non-lagged broadcast path already guaranteed. The durable watermark can never outrun its rows.
Changes
lib.rs: addpersistence_sender/persistence_receiverfields +emit_event.accessors.rs: addtake_persistence_receiver()(handed to the consumer once; buffers pre-drain events, so no subscribe-before-publish race).process_block.rs: route all six emit sites throughemit_event.event_tests.rs: prove a 5000-event burst is delivered losslessly and in order on the persistence channel while the bounded broadcast lags.Ordering / #909
This touches
key-wallet-manager/src/lib.rsandprocess_block.rs, which PR #909 (out-of-order spend) also modifies. This branch is independent of #909 and is based on thedev-ancestor rev the downstream build currently pins. There is expected textual overlap in those two files — stacks on #909, merge after #909 and rebase.Test
cargo test -p key-wallet-managergreen (incl. newpersistence_channel_is_lossless_under_a_large_burst);cargo check -p dash-spvgreen;cargo fmt --checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes