diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs
index df1b4701cf..a3269e2319 100644
--- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs
+++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs
@@ -34,7 +34,7 @@
use std::collections::{BTreeMap, HashMap, HashSet};
use std::sync::atomic::{AtomicBool, Ordering};
-use std::sync::Arc;
+use std::sync::{Arc, Mutex};
use dashcore::blockdata::transaction::{txout::TxOut, OutPoint};
use dashcore::ScriptBuf;
@@ -294,10 +294,16 @@ async fn run_wallet_event_adapter(
P: PlatformWalletPersistence + 'static,
{
tracing::debug!("wallet-event adapter task started");
- let mut fault = AdapterFaultState::default();
+ // Both live behind handles rather than as locals because the commit runs
+ // on a blocking thread (see the `spawn_blocking` below) and has to be able
+ // to carry its state across drains. Moving them into the closure by value
+ // would lose a wallet's frozen watermark if that thread ever panicked —
+ // un-freezing a wallet that failed verification is the one outcome the
+ // fail-closed guard exists to prevent.
+ let fault = Arc::new(Mutex::new(AdapterFaultState::default()));
// One-shot latch so the hard "watermark frozen" line hits logcat exactly
// once per session rather than once per faulted batch.
- let mut freeze_logged = false;
+ let freeze_logged = Arc::new(AtomicBool::new(false));
loop {
// Block for the first event of a batch. Everything already sitting in
@@ -360,14 +366,118 @@ async fn run_wallet_event_adapter
(
// Commit the folded batch. The channel is lossless, so the only way a
// watermark is held back is a rejected `store()` (the fail-closed
// backstop inside `commit_batch`).
- let diag = commit_batch(
- &*persister,
- batch,
- folded,
- &mut fault,
- &sync_fault,
- &mut freeze_logged,
- );
+ // Commit on a blocking thread, never on the async worker.
+ //
+ // `store()` is synchronous and, for the SQLite backend, commits a real
+ // transaction per call — its own docs warn that a slow write blocks
+ // every other wallet accessor for its duration. Called inline here it
+ // blocked a tokio worker instead: a field restore showed one drain of
+ // 512 folded events park the runtime long enough for the metrics tick
+ // covering it to report a 1.4s mean poll, with the whole sync stalled
+ // for minutes at a time and the durable watermark left hundreds of
+ // thousands of blocks behind the chain tip.
+ //
+ // The handle is awaited rather than raced against `cancel`: a store
+ // that has started must be allowed to finish, and dropping the handle
+ // would not stop the thread anyway. Shutdown is observed at the next
+ // `recv` instead.
+ // Captured before the batch moves into the closure: if the commit
+ // thread panics, these are the wallets whose rows have an unknown fate
+ // and whose watermark must therefore be frozen.
+ let batch_wallet_ids: Vec = batch.keys().copied().collect();
+ // Filled by `commit_batch` as each wallet's `store()` returns. Lives
+ // out here so a panicking commit thread cannot take it down with it:
+ // what it holds is the difference between "this wallet's rows are
+ // accounted for" and "nobody knows".
+ let settled: Arc>> = Arc::new(Mutex::new(Vec::new()));
+ let settled_for_commit = Arc::clone(&settled);
+ let persister_for_commit = Arc::clone(&persister);
+ let sync_fault_for_commit = Arc::clone(&sync_fault);
+ let fault_for_commit = Arc::clone(&fault);
+ let freeze_for_commit = Arc::clone(&freeze_logged);
+ let committed = tokio::task::spawn_blocking(move || {
+ // The lock is uncontended by construction — this task is the only
+ // writer, and one drain commits at a time — so it never blocks;
+ // it exists to carry the state, not to arbitrate.
+ let mut fault = fault_for_commit
+ .lock()
+ .unwrap_or_else(|poisoned| poisoned.into_inner());
+ let mut freeze_logged = freeze_for_commit.load(Ordering::Relaxed);
+ let mut settled = settled_for_commit
+ .lock()
+ .unwrap_or_else(|poisoned| poisoned.into_inner());
+ let diag = commit_batch(
+ &*persister_for_commit,
+ batch,
+ folded,
+ &mut fault,
+ &sync_fault_for_commit,
+ &mut freeze_logged,
+ &mut settled,
+ );
+ freeze_for_commit.store(freeze_logged, Ordering::Relaxed);
+ diag
+ })
+ .await;
+
+ let diag = match committed {
+ Ok(diag) => diag,
+ // The commit thread panicked, so `commit_batch` never reached the
+ // `store()` rejection arm that would have frozen the affected
+ // wallets. Freeze them here instead.
+ //
+ // Before this call moved off the runtime a panic unwound the whole
+ // adapter task, which stopped every later watermark advance by
+ // killing the writer. `spawn_blocking` turns that into a recoverable
+ // `JoinError`, and simply continuing would let the NEXT batch
+ // persist a higher `synced_height` for a wallet whose rows from this
+ // batch may never have landed — the exact hole the fail-closed rule
+ // exists to prevent. Faulting per wallet rather than stopping the
+ // adapter keeps the existing design: a wallet whose commit is in
+ // doubt freezes, its siblings keep syncing.
+ Err(join_error) => {
+ // `commit_batch` walks the batch serially, so a panic partitions
+ // it: wallets whose `store()` already returned are settled — their
+ // rows were accepted or rejected, and a rejection already faulted
+ // them from inside. Freezing those too would strip a healthy
+ // wallet's watermark for the rest of the session over a sibling's
+ // bad batch.
+ //
+ // What is left — the wallet that panicked, plus every wallet the
+ // loop never reached — has no known outcome, and its events are
+ // gone from the lossless channel. Those must freeze, or a later
+ // batch advances their watermark past rows that may never have
+ // landed.
+ let settled_ids = settled
+ .lock()
+ .unwrap_or_else(|poisoned| poisoned.into_inner())
+ .iter()
+ .copied()
+ .collect::>();
+ let unsettled: Vec = batch_wallet_ids
+ .iter()
+ .copied()
+ .filter(|id| !settled_ids.contains(id))
+ .collect();
+ {
+ let mut fault = fault
+ .lock()
+ .unwrap_or_else(|poisoned| poisoned.into_inner());
+ for wallet_id in &unsettled {
+ fault.fault_wallet(*wallet_id, &sync_fault);
+ }
+ }
+ tracing::error!(
+ error = %join_error,
+ folded,
+ settled = settled_ids.len(),
+ frozen = unsettled.len(),
+ "wallet-event commit thread failed; freezing the wallets whose \
+ rows have an unknown outcome"
+ );
+ continue;
+ }
+ };
// One structured line per drain via the `log` facade so a tester
// logcat is unambiguous about whether the watermark is advancing.
@@ -411,6 +521,7 @@ fn commit_batch(
fault: &mut AdapterFaultState,
sync_fault: &AtomicBool,
freeze_logged: &mut bool,
+ settled: &mut Vec,
) -> BatchDiagnostics
where
P: PlatformWalletPersistence + ?Sized,
@@ -459,7 +570,15 @@ where
asset_locks: (!Merge::is_empty(&asset_locks)).then_some(asset_locks),
..PlatformWalletChangeSet::default()
};
- match persister.store(wallet_id, cs) {
+ let store_result = persister.store(wallet_id, cs);
+ // Recorded only once the store has RETURNED, and whether it accepted
+ // or rejected — both are answers. A wallet whose store panicked never
+ // reaches this line, and one the loop never got to is never pushed at
+ // all, so what is missing from `settled` is exactly the set whose
+ // outcome the caller cannot reason about. See the `JoinError` arm in
+ // `run_wallet_event_adapter`.
+ settled.push(wallet_id);
+ match store_result {
Ok(()) => {
if let Some(h) = offered_height {
diag.record_persisted(h);
@@ -1880,6 +1999,17 @@ mod tests {
struct ProbePersister {
obs: UnboundedSender,
fail_once: Mutex>,
+ /// Wallets whose NEXT `store()` panics instead of returning. Models a
+ /// backend that dies mid-write — the case that used to unwind the whole
+ /// adapter task and now surfaces as a `JoinError`.
+ panic_once: Mutex>,
+ /// Held closed to keep a `store()` call parked. The SQLite backend
+ /// commits a real transaction per call, so a slow disk parks the caller
+ /// for real; this makes that duration controllable.
+ block_until: Mutex>>,
+ /// Raised as soon as a blocked `store()` is entered, so a test can wait
+ /// for the block to be in effect rather than sleeping and hoping.
+ blocked: Arc,
}
impl ProbePersister {
@@ -1887,11 +2017,24 @@ mod tests {
Self {
obs,
fail_once: Mutex::new(HashSet::new()),
+ panic_once: Mutex::new(HashSet::new()),
+ block_until: Mutex::new(None),
+ blocked: Arc::new(AtomicBool::new(false)),
}
}
+ /// Park the next `store()` until the returned sender is dropped or
+ /// signalled. `blocked` reports when the park is actually in effect.
+ fn block_next(&self) -> (std::sync::mpsc::Sender<()>, Arc) {
+ let (tx, rx) = std::sync::mpsc::channel();
+ *self.block_until.lock().unwrap() = Some(rx);
+ (tx, Arc::clone(&self.blocked))
+ }
fn fail_next(&self, wallet_id: WalletId) {
self.fail_once.lock().unwrap().insert(wallet_id);
}
+ fn panic_next(&self, wallet_id: WalletId) {
+ self.panic_once.lock().unwrap().insert(wallet_id);
+ }
}
impl PlatformWalletPersistence for ProbePersister {
@@ -1901,6 +2044,17 @@ mod tests {
changeset: PlatformWalletChangeSet,
) -> Result<(), PersistenceError> {
let core = changeset.core.as_ref();
+ if let Some(gate) = self.block_until.lock().unwrap().take() {
+ self.blocked.store(true, Ordering::Relaxed);
+ // Blocks the calling thread outright — the whole point is to
+ // model a synchronous backend, so an async wait would prove
+ // nothing.
+ let _ = gate.recv();
+ self.blocked.store(false, Ordering::Relaxed);
+ }
+ if self.panic_once.lock().unwrap().remove(&wallet_id) {
+ panic!("probe persister: store panicked for {wallet_id:?}");
+ }
let rejected = self.fail_once.lock().unwrap().remove(&wallet_id);
let _ = self.obs.send(StoreObserved {
wallet_id,
@@ -2253,6 +2407,243 @@ mod tests {
);
}
+ /// (h) SAFETY INVARIANT under a commit-thread PANIC: a wallet whose
+ /// `store()` panicked must be frozen just as if the store had been
+ /// rejected, because its rows have an unknown fate.
+ ///
+ /// This is a regression guard on the move to `spawn_blocking`. Before it,
+ /// a panic unwound the adapter task itself, which stopped every later
+ /// watermark advance by killing the writer outright. `spawn_blocking`
+ /// turns that into a recoverable `JoinError` — and merely logging it would
+ /// let the NEXT batch persist a higher `synced_height` for a wallet whose
+ /// earlier rows may never have landed, which is exactly the hole
+ /// dashpay/platform#4069 closed.
+ #[tokio::test]
+ async fn a_panicking_commit_freezes_the_batch_wallets() {
+ let wallet_id = [0xEEu8; 32];
+ let (tx, rx) = unbounded_channel::();
+
+ let (obs_tx, mut obs_rx) = unbounded_channel();
+ let persister = Arc::new(ProbePersister::new(obs_tx));
+ persister.panic_next(wallet_id);
+ let sync_fault = Arc::new(AtomicBool::new(false));
+ let cancel = CancellationToken::new();
+ let handle = tokio::spawn(run_wallet_event_adapter(
+ test_manager(),
+ Arc::clone(&persister),
+ rx,
+ Arc::clone(&sync_fault),
+ cancel.clone(),
+ ));
+
+ // The store for this batch panics: no observation is emitted, and the
+ // adapter must fault the wallet rather than carry on unaffected.
+ tx.send(block_processed_event(wallet_id, 10)).unwrap();
+ // Bounded, so a regression fails the test instead of hanging it: with
+ // the fault-on-panic path removed, `sync_fault` is simply never raised
+ // and an unbounded spin would wedge CI with no diagnosis.
+ tokio::time::timeout(std::time::Duration::from_secs(5), async {
+ while !sync_fault.load(Ordering::Relaxed) {
+ tokio::task::yield_now().await;
+ }
+ })
+ .await
+ .expect("a panicked commit must raise the hard-fault signal");
+
+ // The adapter must still be alive — the point of moving the commit off
+ // the runtime is that one bad batch does not take the writer with it.
+ assert!(
+ !handle.is_finished(),
+ "a panicked commit must not kill the adapter"
+ );
+
+ // A later watermark for the same wallet must not reach the store.
+ tx.send(block_processed_event(wallet_id, 60)).unwrap();
+ tx.send(sync_height_event(wallet_id, 900)).unwrap();
+
+ let post = tokio::time::timeout(std::time::Duration::from_secs(5), obs_rx.recv())
+ .await
+ .expect("a faulted wallet must still persist its rows")
+ .expect("the record-bearing event must still persist while faulted");
+ assert_eq!(post.wallet_id, wallet_id);
+ assert_eq!(
+ post.synced_height, None,
+ "a wallet whose commit panicked must not advance its durable watermark"
+ );
+
+ cancel.cancel();
+ drop(tx);
+ handle.await.unwrap();
+ }
+
+ /// (j) THE PRIMARY BEHAVIOUR OF THIS PR: a blocked `store()` must not park
+ /// the async runtime.
+ ///
+ /// `store()` is synchronous and, for the SQLite backend, commits a real
+ /// transaction per call. Called inline on a tokio worker it held that
+ /// worker for the duration — a field restore showed one drain park the
+ /// runtime long enough for the metrics tick covering it to report a 1.4s
+ /// mean poll, with the durable watermark left hundreds of thousands of
+ /// blocks behind the chain tip.
+ ///
+ /// Every other test in this module would still pass with `commit_batch`
+ /// moved back inline, because they only check persistence outcomes. This
+ /// one runs the adapter on a SINGLE worker, parks a `store()`, and requires
+ /// a spawned task to still get scheduled.
+ ///
+ /// Deliberately built out of `std` primitives — a `std::mpsc` handoff and
+ /// `std::thread::sleep` on the test thread — rather than `tokio::time`.
+ /// The regression parks the runtime's only worker, and a tokio timer needs
+ /// that runtime to fire: an async timeout here hangs instead of failing,
+ /// which is worse than the bug it is meant to catch.
+ #[test]
+ fn a_blocked_store_does_not_park_the_runtime() {
+ use std::sync::mpsc as std_mpsc;
+ use std::time::{Duration, Instant};
+
+ let runtime = tokio::runtime::Builder::new_multi_thread()
+ .worker_threads(1)
+ .enable_all()
+ .build()
+ .unwrap();
+
+ let wallet_id = [0x33u8; 32];
+ let (tx, rx) = unbounded_channel::();
+ let (obs_tx, mut obs_rx) = unbounded_channel();
+ let persister = Arc::new(ProbePersister::new(obs_tx));
+ let (release, blocked) = persister.block_next();
+ let sync_fault = Arc::new(AtomicBool::new(false));
+ let cancel = CancellationToken::new();
+
+ let handle = runtime.spawn(run_wallet_event_adapter(
+ test_manager(),
+ Arc::clone(&persister),
+ rx,
+ Arc::clone(&sync_fault),
+ cancel.clone(),
+ ));
+
+ // Park the commit inside a synchronous `store()`.
+ tx.send(block_processed_event(wallet_id, 10)).unwrap();
+ let deadline = Instant::now() + Duration::from_secs(5);
+ while !blocked.load(Ordering::Relaxed) {
+ assert!(
+ Instant::now() < deadline,
+ "the store must actually park before the assertion below means anything"
+ );
+ std::thread::sleep(Duration::from_millis(10));
+ }
+
+ // The discriminator: a SPAWNED task has to be scheduled on the
+ // runtime's single worker. With the commit on `spawn_blocking` the
+ // worker is free and this arrives at once; with it inline the worker is
+ // sitting inside `store()` and this times out.
+ let (sentinel_tx, sentinel_rx) = std_mpsc::channel();
+ runtime.spawn(async move {
+ let _ = sentinel_tx.send(());
+ });
+ sentinel_rx
+ .recv_timeout(Duration::from_secs(5))
+ .expect("a blocked store must not hold the runtime's only worker");
+
+ // Release, then let the drain finish so the adapter shuts down cleanly.
+ drop(release);
+ runtime.block_on(async {
+ let observed = tokio::time::timeout(Duration::from_secs(5), obs_rx.recv())
+ .await
+ .expect("the released store must complete")
+ .expect("store observed");
+ assert_eq!(observed.wallet_id, wallet_id);
+
+ cancel.cancel();
+ drop(tx);
+ handle.await.unwrap();
+ });
+ }
+
+ /// (i) A commit panic must not punish the wallets it did not reach.
+ ///
+ /// `commit_batch` walks the batch serially (a `BTreeMap`, so in wallet-id
+ /// order). If an earlier wallet's `store()` returned and a later one
+ /// panics, the earlier wallet's rows are on disk and its watermark is
+ /// safe — freezing it would strip its `synced_height` for the rest of the
+ /// session over a sibling's bad batch.
+ ///
+ /// Guards the fix for the first version of the panic handler, which
+ /// faulted every wallet in the drain.
+ #[tokio::test]
+ async fn a_panicking_commit_spares_the_wallets_it_already_stored() {
+ // `BTreeMap` order decides who is committed first, so the ids are
+ // chosen to put the healthy wallet ahead of the panicking one.
+ let healthy = [0x11u8; 32];
+ let doomed = [0x22u8; 32];
+ let (tx, rx) = unbounded_channel::();
+
+ let (obs_tx, mut obs_rx) = unbounded_channel();
+ let persister = Arc::new(ProbePersister::new(obs_tx));
+ persister.panic_next(doomed);
+ let sync_fault = Arc::new(AtomicBool::new(false));
+ let cancel = CancellationToken::new();
+ let handle = tokio::spawn(run_wallet_event_adapter(
+ test_manager(),
+ Arc::clone(&persister),
+ rx,
+ Arc::clone(&sync_fault),
+ cancel.clone(),
+ ));
+
+ // Both wallets in one drain: `healthy` stores, then `doomed` panics.
+ // Sent before either is observed so they fold into a single batch.
+ tx.send(block_processed_event(healthy, 10)).unwrap();
+ tx.send(block_processed_event(doomed, 10)).unwrap();
+
+ tokio::time::timeout(std::time::Duration::from_secs(5), async {
+ while !sync_fault.load(Ordering::Relaxed) {
+ tokio::task::yield_now().await;
+ }
+ })
+ .await
+ .expect("the panicked commit must raise the hard-fault signal");
+
+ // Drain what the panicking batch managed to observe.
+ while obs_rx.try_recv().is_ok() {}
+
+ // The healthy wallet's watermark must still advance: its store
+ // returned, so its rows are accounted for.
+ tx.send(sync_height_event(healthy, 900)).unwrap();
+ // Bounded: on a regression the healthy wallet is frozen, its
+ // watermark-only changeset collapses to nothing, and no store is
+ // observed at all — an unbounded `recv` would hang CI instead of
+ // reporting which invariant broke.
+ let after_healthy = tokio::time::timeout(std::time::Duration::from_secs(5), obs_rx.recv())
+ .await
+ .expect("a wallet whose store completed must still be storable")
+ .expect("healthy wallet still stores");
+ assert_eq!(after_healthy.wallet_id, healthy);
+ assert_eq!(
+ after_healthy.synced_height,
+ Some(900),
+ "a wallet whose store completed must not be frozen by a sibling's panic"
+ );
+
+ // The wallet whose store panicked must be frozen.
+ tx.send(block_processed_event(doomed, 60)).unwrap();
+ tx.send(sync_height_event(doomed, 900)).unwrap();
+ let after_doomed = tokio::time::timeout(std::time::Duration::from_secs(5), obs_rx.recv())
+ .await
+ .expect("a faulted wallet must still persist its rows")
+ .expect("doomed wallet still persists rows");
+ assert_eq!(after_doomed.wallet_id, doomed);
+ assert_eq!(
+ after_doomed.synced_height, None,
+ "the wallet whose commit panicked must not advance its watermark"
+ );
+
+ cancel.cancel();
+ drop(tx);
+ handle.await.unwrap();
+ }
+
/// (g) SAFETY INVARIANT under a fault: once the per-wallet fault latch is
/// set (here by a rejected `store()`), no later changeset for that wallet
/// may advance the durable `synced_height` — whether the watermark arrives
@@ -2671,6 +3062,7 @@ mod tests {
&mut fault,
&sync_fault,
&mut freeze_logged,
+ &mut Vec::new(),
);
let observed = obs_rx
@@ -2743,6 +3135,7 @@ mod tests {
&mut fault,
&sync_fault,
&mut freeze_logged,
+ &mut Vec::new(),
);
assert_eq!(diag.persisted, Some(500));
@@ -2774,6 +3167,7 @@ mod tests {
&mut fault,
&sync_fault,
&mut freeze_logged,
+ &mut Vec::new(),
);
// The height was genuinely offered to the store...
@@ -2843,6 +3237,7 @@ mod tests {
&mut fault,
&sync_fault,
&mut freeze_logged,
+ &mut Vec::new(),
);
assert_eq!(diag.persisted, None, "a frozen watermark is not persisted");
@@ -2890,6 +3285,7 @@ mod tests {
&mut fault,
&sync_fault,
&mut freeze_logged,
+ &mut Vec::new(),
);
assert_eq!(diag.frozen, Some(1234));
@@ -2926,6 +3322,7 @@ mod tests {
&mut fault,
&sync_fault,
&mut freeze_logged,
+ &mut Vec::new(),
);
assert_eq!(
@@ -2969,6 +3366,7 @@ mod tests {
&mut fault,
&sync_fault,
&mut freeze_logged,
+ &mut Vec::new(),
);
assert_eq!(diag.wallets, 1);