fix(sync): add cooperative hard-cancel support

[dolcalmi]

Summary

• add cooperative sync cancellation controls (SyncControl) to wallet setup and sync trait paths while preserving existing defaults
• add Electrum checkpoint cancellation handling and return explicit Error::SyncCancelled on controlled interruption
• integrate sync_control into SyncOptions, add cancellation propagation tests (including setup path), and bump crate version to 0.30.4

[Copilot]

Pull request overview

This PR introduces cooperative “hard-cancel” support for wallet sync by threading a new SyncControl (cancellation flag + optional deadline) through Wallet::sync into blockchain backends, and by surfacing a dedicated Error::SyncCancelled when sync is interrupted.

Changes:

• Add SyncControl and propagate it via SyncOptions and new WalletSync::{wallet_setup_with_control, wallet_sync_with_control} defaulted trait methods.
• Implement cancellation checkpoints in the Electrum backend and return Error::SyncCancelled on cancellation.
• Add wallet sync cancellation tests and bump crate version to 0.30.4.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/wallet/mod.rs	Extends SyncOptions, propagates SyncControl through sync/setup calls, adds sync cancellation tests.
src/blockchain/mod.rs	Introduces SyncControl and adds new WalletSync “with_control” methods with default behavior.
src/blockchain/electrum.rs	Adds cancellation checkpoints during Electrum sync/setup and propagates control into tx fetch caching.
src/blockchain/any.rs	Forwards the new “with_control” trait methods through AnyBlockchain.
src/error.rs	Adds Error::SyncCancelled and its Display string.
Cargo.toml	Bumps crate version to 0.30.4.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

SyncControl::is_cancelled uses Option::is_some_and, but the crate MSRV is Rust 1.63 (Cargo.toml sets rust-version = "1.63"). is_some_and was stabilized later, so this will fail to compile on the declared MSRV. Replace these calls with an MSRV-compatible pattern (e.g., as_ref().map_or(false, |v| ...) / matches!).

Suggested change

	.is_some_and(|cancel| cancel.load(Ordering::Relaxed))
	|| self
	.deadline
	.is_some_and(|deadline| Instant::now() >= deadline)
	.map_or(false, |cancel| cancel.load(Ordering::Relaxed))
	|| self
	.deadline
	.map_or(false, |deadline| Instant::now() >= deadline)

[Copilot]

This test is intended to exercise the wallet_setup*_with_control path, but it constructs the wallet with get_test_wpkh() (non-derivable descriptor, no /*). In Wallet::sync, run_setup is driven by ensure_addresses_cached, which will be false for non-derivable descriptors, so the test will actually hit the wallet_sync*_with_control path again. Use a derivable descriptor (one with a wildcard) so ensure_addresses_cached returns true on first sync and the setup path is actually exercised.

Suggested change

	let wallet = Wallet::new(
	get_test_wpkh(),
	let derivable_descriptor = get_test_wpkh().replace("/0)", "/*)");
	let wallet = Wallet::new(
	&derivable_descriptor,

[Copilot]

The inherent helper ElectrumBlockchain::wallet_setup_with_control has the same name as the new WalletSync::wallet_setup_with_control trait method. Although it currently resolves due to signature differences, this naming collision is easy to misread and can lead to accidental recursion/incorrect dispatch if signatures change. Consider renaming the inherent helper (e.g., wallet_setup_impl) or using explicit UFCS in the trait impl to make the intended target unambiguous.

Suggested change

	fn wallet_setup_with_control<D: BatchDatabase>(
	fn wallet_setup_impl<D: BatchDatabase>(

[dolcalmi]

Addressed Copilot feedback in follow-up commit:

• Replaced is_some_and with MSRV-compatible map_or usage in SyncControl::is_cancelled
• Renamed electrum helper to wallet_setup_impl to avoid trait/inherent naming collision
• Fixed setup-path cancellation test to use a derivable wildcard descriptor so run_setup is actually exercised

All library tests still pass (rtk cargo test --lib).

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The inline comment still refers to sync_opts.progress, but sync_opts is destructured above and the variable is now progress. This comment is now misleading; update it to reflect the current variable name/flow.

Suggested change

	// TODO: for the next runs, we cannot reuse the `sync_opts.progress` object due to trait
	// TODO: for the next runs, we cannot reuse the `progress` object due to trait

[Copilot]

test_sync_with_control_uses_wallet_sync_with_control appears to assume Wallet::sync() always goes through wallet_sync_with_control, but the first sync on a fresh wallet typically takes the setup path (run_setup becomes true after caching addresses) and will call wallet_setup_with_control. With the current CountingBlockchain counters, setup_calls will be incremented on that first call, so these assertions likely fail. Consider either pre-caching addresses before the first sync (so run_setup is false), or splitting the test into separate setup-vs-sync expectations with distinct counters for wallet_setup_with_control vs wallet_setup.

[Copilot]

Adding the new public sync_control field to SyncOptions is a source-breaking API change for downstream users constructing SyncOptions with struct literals (they must now provide sync_control or use ..Default::default()). If this crate intends patch-level compatibility, consider an alternative that doesn’t change the public struct shape (e.g., keep SyncOptions unchanged and add a separate sync_with_control parameter only, or provide a constructor/helper so most callers don’t use struct literals), or bump the crate version according to your breaking-change policy.

Suggested change

	/// Optional cooperative control for cancellation and deadlines.
	pub sync_control: Option<SyncControl>,
	}
	}
	impl SyncOptions {
	/// Create default sync options.
	pub fn new() -> Self {
	Self::default()
	}
	/// Create sync options with the provided progress tracker.
	pub fn with_progress(progress: Option<Box<dyn Progress>>) -> Self {
	Self { progress }
	}
	}

[Copilot]

Error is a public, non-#[non_exhaustive] enum; adding the new SyncCancelled variant is a source-breaking change for downstream code that exhaustively matches on Error. If this crate aims to avoid breaking changes in a patch release, consider a non-breaking representation (e.g., reusing an existing variant, or making Error non-exhaustive in a major/minor bump) and/or adjust the versioning accordingly.

[dolcalmi]

Addressed the additional Copilot follow-up comments:

• Updated stale inline comment in Wallet::sync (sync_opts.progress -> progress)
• Clarified test bookkeeping by renaming setup counter to setup_with_control_calls
• Kept assertions aligned with intended setup-vs-sync behavior for the mocked blockchain

Re-ran tests: rtk cargo test --lib (all passing).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/wallet/mod.rs:1835

• SyncControl is currently only passed into the blockchain backend; Wallet::sync itself never checks control.check_cancelled(). This means cancellation can be requested and the wallet may still spend time caching addresses and/or call get_height + persist sync_time before returning. Consider adding cancellation checkpoints in Wallet::sync (e.g., at the start, between rounds/address-caching, and before saving sync_time) so cancellation behaves consistently regardless of backend checkpoints.

        let SyncOptions {
            progress,
            sync_control,
        } = sync_opts;
        let control = sync_control.unwrap_or_default();

        debug!("Begin sync...");

        // TODO: for the next runs, we cannot reuse the `progress` object due to trait
        // restrictions
        let mut progress_iter = progress.into_iter();
        let mut new_progress = || {
            progress_iter
                .next()
                .unwrap_or_else(|| Box::new(NoopProgress))
        };

        let run_setup = self.ensure_addresses_cached(CACHE_ADDR_BATCH_SIZE)?;
        debug!("run_setup: {}", run_setup);

        // TODO: what if i generate an address first and cache some addresses?
        // TODO: we should sync if generating an address triggers a new batch to be stored

        // We need to ensure descriptor is derivable to fullfil "missing cache", otherwise we will
        // end up with an infinite loop
        let has_wildcard = self.descriptor.has_wildcard()
            && (self.change_descriptor.is_none()
                || self.change_descriptor.as_ref().unwrap().has_wildcard());

        // Restrict max rounds in case of faulty "missing cache" implementation by blockchain
        let max_rounds = if has_wildcard { 100 } else { 1 };

        for _ in 0..max_rounds {
            let sync_res = if run_setup {
                maybe_await!(blockchain.wallet_setup_with_control(
                    &self.database,
                    new_progress(),
                    control.clone(),
                ))
            } else {
                maybe_await!(blockchain.wallet_sync_with_control(
                    &self.database,
                    new_progress(),
                    control.clone(),
                ))
            };

            // If the error is the special `MissingCachedScripts` error, we return the number of
            // scripts we should ensure cached.
            // On any other error, we should return the error.
            // On no error, we say `ensure_cache` is 0.
            let ensure_cache = sync_res.map_or_else(
                |e| match e {
                    Error::MissingCachedScripts(inner) => {
                        // each call to `WalletSync` is expensive, maximize on scripts to search for
                        let extra =
                            std::cmp::max(inner.missing_count as u32, CACHE_ADDR_BATCH_SIZE);
                        let last = inner.last_count as u32;
                        Ok(extra + last)
                    }
                    _ => Err(e),
                },
                |_| Ok(0_u32),
            )?;

            // cache and try again, break when there is nothing to cache
            if !self.ensure_addresses_cached(ensure_cache)? {
                break;
            }
        }

        let sync_time = SyncTime {
            block_time: BlockTime {
                height: maybe_await!(blockchain.get_height())?,
                timestamp: time::get_timestamp(),
            },
        };
        debug!("Saving `sync_time` = {:?}", sync_time);
        self.database.borrow_mut().set_sync_time(sync_time)?;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Adding a new public field to SyncOptions is a breaking change for downstream users that construct it with a struct literal (they will now fail to compile). If you want this to be a non-breaking release, consider keeping SyncOptions unchanged and exposing cooperative cancellation via the new Wallet::sync_with_control API (or introduce a separate options type / builder) rather than extending this public struct.

Suggested change

	pub progress: Option<Box<dyn Progress>>,
	pub progress: Option<Box<dyn Progress>>,
	}
	#[derive(Debug, Default)]
	/// Options to a controlled [`sync`] operation.
	///
	/// This type can be used by APIs that support cooperative cancellation
	/// and deadlines without changing the public shape of [`SyncOptions`].
	///
	/// [`sync`]: Wallet::sync
	pub struct SyncWithControlOptions {
	/// The progress tracker which may be informed when progress is made.
	pub progress: Option<Box<dyn Progress>>,

[Copilot]

The crate version is being bumped as a patch release, but this PR also changes the public API in a potentially breaking way (e.g., SyncOptions gained a new field). If this is intended to be non-breaking, please avoid breaking public structs or bump the version according to the project's semver policy.

Suggested change

	version = "0.30.4"
	version = "0.31.0"

[openoms]

@blink-claw-bot review this, focus on safety

[blink-claw-bot]

Safety-Focused Review: Cooperative Sync Cancellation

Overall this is a well-designed cooperative cancellation mechanism. The approach of checkpoint-based polling (check_cancelled() at natural loop boundaries) is the right pattern for safe cancellation in Rust. A few safety observations:

✅ What's solid

• No partial writes on cancel. The cancellation check before database.commit_batch() in electrum.rs:258 is critical — this ensures a cancelled sync never commits a partial batch to the database. Good.
• Ordering::Relaxed is correct for the AtomicBool cancel flag. There's no data dependency between the flag and other shared state — you just need the store to eventually be visible, which Relaxed guarantees.
• Default trait impls fall through to the existing non-cancellable paths, so backends that don't implement _with_control are completely unaffected. No breaking changes.
• SyncCancelled error variant is explicit and matchable — callers can distinguish cancellation from real failures.

⚠️ Safety observations

1. SyncControl::clone() in the sync loop (wallet/mod.rs): SyncControl derives Clone, which clones the Arc<AtomicBool> (cheap, shares the flag) and copies the Instant deadline. This is fine, but worth noting that cloning doesn't create independent controls — all clones share the same cancel flag. This is the desired behavior but should be documented.

2. Deadline precision is best-effort. The deadline check happens only at checkpoints, so actual cancellation may overshoot by the duration of one Electrum RPC call (batch_script_list_unspent, batch_transaction_get, etc). For a "hard cancel" feature name, users should understand this is cooperative, not preemptive. The doc comments are clear about this — good.

3. TxCache::save_txs cancellation granularity. The cancel check inside the for txid in txids loop (line ~310) means individual tx fetches from the DB can be interrupted. But the batch_transaction_get call in the chunk loop below is where the real time is spent — if Electrum is slow, that single RPC can block for a while before the next checkpoint. This is acceptable for cooperative cancellation but worth a comment.

4. No cancellation check after batch_transaction_get. In save_txs, after self.client.batch_transaction_get(chunk) returns, there's no check_cancelled() before inserting into the cache. Not a safety issue (cache is ephemeral), but a missed opportunity for faster response to cancellation.

5. Version bump 0.30.2 → 0.30.4 (skipping 0.30.3). Minor, but is this intentional? The base branch is pr-base-v0.30.3.

💡 Suggestion

Consider adding a brief doc comment on SyncControl noting that cloned instances share the same cancellation flag (point 1 above). This helps consumers who might expect independent controls from .clone().

Verdict

Safe for production. The cancellation is cooperative and conservative — it errs on the side of completing in-flight work before checking. No risk of data corruption or partial state. The only "worst case" is that cancellation takes slightly longer than expected due to in-flight RPC calls, which is the correct tradeoff.

🤖 Review by claude-opus-4.6 via Blink-Claw-Bot