Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 63 additions & 11 deletions packages/rs-platform-wallet/src/changeset/core_bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -294,10 +294,16 @@ async fn run_wallet_event_adapter<P>(
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
Expand Down Expand Up @@ -360,14 +366,60 @@ async fn run_wallet_event_adapter<P>(
// 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.
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 diag = commit_batch(
&*persister_for_commit,
batch,
folded,
&mut fault,
&sync_fault_for_commit,
&mut freeze_logged,
);
freeze_for_commit.store(freeze_logged, Ordering::Relaxed);
diag
})
.await;
Comment on lines +392 to +411

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: The off-runtime persistence boundary has no regression test

The existing ProbePersister returns immediately and checks only persistence outcomes. Those tests still pass if commit_batch is moved back inline onto the Tokio worker, so they do not protect the primary behavior introduced by this PR. Add a controllably blocking persister and run the adapter on a current-thread or single-worker runtime, then verify that another future makes progress while store() remains blocked. The fixture should also cover a panicking store and assert that no later store for the affected wallet carries synced_height.

source: ['codex']


let diag = match committed {
Ok(diag) => diag,
// The commit thread panicked. The fault state survives (it lives
// behind the handle above), but this batch's outcome is unknown,
// so it is reported rather than silently folded into the next one.
Err(join_error) => {
tracing::error!(
error = %join_error,
folded,
"wallet-event commit thread failed; batch outcome unknown"
);
continue;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +413 to +444

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Continuing after a commit panic can advance the watermark past lost rows

commit_batch consumes the folded batch and calls store() for each wallet. If store() panics before its outcome is known, unwinding bypasses the Err arm that calls fault_wallet, drops the remainder of the consumed batch, and returns a JoinError. This branch then continues with an unaffected fault state, so a later event for the same wallet can successfully persist a higher synced_height even though rows from the panicked batch may be absent. That violates the adapter's fail-closed invariant; before this PR, the panic terminated the adapter and prevented later watermark advancement. Stop the adapter and latch sync_fault, or capture every batch wallet ID before moving the batch and fault all of them before continuing.

Suggested change
let diag = match committed {
Ok(diag) => diag,
// The commit thread panicked. The fault state survives (it lives
// behind the handle above), but this batch's outcome is unknown,
// so it is reported rather than silently folded into the next one.
Err(join_error) => {
tracing::error!(
error = %join_error,
folded,
"wallet-event commit thread failed; batch outcome unknown"
);
continue;
Err(join_error) => {
tracing::error!(
error = %join_error,
folded,
"wallet-event commit thread failed; stopping adapter because batch outcome is unknown"
);
sync_fault.store(true, Ordering::Relaxed);
break;
}

source: ['codex']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0499b9c9 — and you are right that this PR introduced the hole rather than merely failing to close it.

Before the move, a panic inside store() unwound the adapter task itself. Violent, but safe in one specific way: the writer was gone, so nothing could persist a higher synced_height afterwards. spawn_blocking converts the same panic into a recoverable JoinError, and my branch logged it and continued with the fault state untouched — so the next batch could advance the watermark past rows whose fate is unknown. Exactly what #4069 closed.

The batch's wallet ids are now captured before the batch moves into the closure, and a JoinError faults every one of them.

On stopping the adapter versus faulting per wallet — I took the per-wallet route rather than the break you suggested. Reasoning, and I am happy to be overruled: a rejected store() already freezes only the wallet it affected and lets its siblings keep syncing, and stopping the adapter would freeze every wallet on the manager over one wallet's bad batch. It would also undo the reason the commit moved off the runtime — one bad batch would again take down the writer for the whole session, just via a different mechanism. sync_fault is latched either way, so the host still sees the hard fault. If you would rather have the harder guarantee, say so and I will switch it to break.

The test covers all three halves of the contract: the hard-fault signal is raised, the adapter survives, and no later store for that wallet carries a synced_height.

On the second point — the off-runtime boundary having no regression coverage — this is a partial answer, not a complete one. ProbePersister gained a panic_next mode, which covers the panicking-store half you asked for. What it does not yet cover is the blocking half: a controllably-blocking persister on a single-worker runtime, asserting another future makes progress while store() is held. That is the test that would actually fail if someone moved commit_batch back inline, and it is worth having. I would rather add it as a follow-up than bolt a timing-sensitive fixture on at the end of this PR — but if you want it here before merge, I will write it.

One note on the test's shape, since it looks over-engineered otherwise: the wait for sync_fault is bounded by an explicit timeout. My first version spun unbounded, which meant a regression hung CI instead of failing it. With the fault path removed the test now fails in 5s with "a panicked commit must raise the hard-fault signal" — verified by actually removing it.

cargo test -p platform-wallet --lib   # 663 passed
cargo clippy --all-targets + cargo fmt --check   # clean

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 0499b9cContinuing after a commit panic can advance the watermark past lost rows no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

}
};

// One structured line per drain via the `log` facade so a tester
// logcat is unambiguous about whether the watermark is advancing.
Expand Down
Loading