From 425e4941a64b56aec8a3964daacc92349106c683 Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Mon, 20 Apr 2026 16:01:51 +0800 Subject: [PATCH 1/7] refactor: extract _refresh() body into _doRefreshWork helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure refactor — no behaviour change. _refresh() is now focused on mutex acquisition, event firing, and error/completion handling; the sequenced work lives in _doRefreshWork(viewOnly). This lets subsequent changes (e.g. wrapping the work in a watchdog) touch only the helper call site instead of forcing a large indentation shift of the entire body. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/wallets/wallet/wallet.dart | 180 +++++++++++++++++---------------- 1 file changed, 92 insertions(+), 88 deletions(-) diff --git a/lib/wallets/wallet/wallet.dart b/lib/wallets/wallet/wallet.dart index 1aa40ef6a..daaf017d5 100644 --- a/lib/wallets/wallet/wallet.dart +++ b/lib/wallets/wallet/wallet.dart @@ -641,113 +641,117 @@ abstract class Wallet { ); } - // add some small buffer before making calls. - // this can probably be removed in the future but was added as a - // debugging feature - await Future.delayed(const Duration(milliseconds: 300)); - - // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. - final Set codesToCheck = {}; - if (this is PaynymInterface && !viewOnly) { - // isSegwit does not matter here at all - final myCode = await (this as PaynymInterface).getPaymentCode( - isSegwit: false, - ); + await _doRefreshWork(viewOnly); - final nym = await PaynymIsApi().nym(myCode.toString()); - if (nym.value != null) { - for (final follower in nym.value!.followers) { - codesToCheck.add(follower.code); - } - for (final following in nym.value!.following) { - codesToCheck.add(following.code); - } - } + completer.complete(); + } catch (error, strace) { + completer.completeError(error, strace); + } finally { + refreshMutex.release(); + if (!completer.isCompleted) { + completer.completeError( + "finally block hit before completer completed", + StackTrace.current, + ); } - _fireRefreshPercentChange(0); - await updateChainHeight(); - - if (this is BitcoinFrostWallet) { - await (this as BitcoinFrostWallet).lookAhead(); - } + Logging.instance.i( + "Refresh for " + "$walletId::${info.name}: ${DateTime.now().difference(start)}", + ); + } + } - _fireRefreshPercentChange(0.1); + Future _doRefreshWork(bool viewOnly) async { + // add some small buffer before making calls. + // this can probably be removed in the future but was added as a + // debugging feature + await Future.delayed(const Duration(milliseconds: 300)); + + // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. + final Set codesToCheck = {}; + if (this is PaynymInterface && !viewOnly) { + // isSegwit does not matter here at all + final myCode = await (this as PaynymInterface).getPaymentCode( + isSegwit: false, + ); - // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. - if (this is MultiAddressInterface) { - if (info.otherData[WalletInfoKeys.reuseAddress] != true) { - await (this as MultiAddressInterface) - .checkReceivingAddressForTransactions(); + final nym = await PaynymIsApi().nym(myCode.toString()); + if (nym.value != null) { + for (final follower in nym.value!.followers) { + codesToCheck.add(follower.code); + } + for (final following in nym.value!.following) { + codesToCheck.add(following.code); } } + } - _fireRefreshPercentChange(0.2); + _fireRefreshPercentChange(0); + await updateChainHeight(); - // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. - if (this is MultiAddressInterface) { - if (info.otherData[WalletInfoKeys.reuseAddress] != true) { - await (this as MultiAddressInterface) - .checkChangeAddressForTransactions(); - } - } - _fireRefreshPercentChange(0.3); - if (this is SparkInterface && !viewOnly) { - // this should be called before updateTransactions() - await (this as SparkInterface).refreshSparkData((0.3, 0.6)); - } + if (this is BitcoinFrostWallet) { + await (this as BitcoinFrostWallet).lookAhead(); + } - if (this is NamecoinWallet) { - await updateUTXOs(); - _fireRefreshPercentChange(0.6); - await (this as NamecoinWallet).checkAutoRegisterNameNewOutputs(); - _fireRefreshPercentChange(0.70); - await updateTransactions(); - } else { - final fetchFuture = updateTransactions(); - _fireRefreshPercentChange(0.6); - final utxosRefreshFuture = updateUTXOs(); - _fireRefreshPercentChange(0.65); - await utxosRefreshFuture; - _fireRefreshPercentChange(0.70); - await fetchFuture; + _fireRefreshPercentChange(0.1); + + // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. + if (this is MultiAddressInterface) { + if (info.otherData[WalletInfoKeys.reuseAddress] != true) { + await (this as MultiAddressInterface) + .checkReceivingAddressForTransactions(); } + } - // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. - if (!viewOnly && this is PaynymInterface && codesToCheck.isNotEmpty) { - await (this as PaynymInterface).checkForNotificationTransactionsTo( - codesToCheck, - ); - // check utxos again for notification outputs - await updateUTXOs(); + _fireRefreshPercentChange(0.2); + + // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. + if (this is MultiAddressInterface) { + if (info.otherData[WalletInfoKeys.reuseAddress] != true) { + await (this as MultiAddressInterface) + .checkChangeAddressForTransactions(); } - _fireRefreshPercentChange(0.80); + } + _fireRefreshPercentChange(0.3); + if (this is SparkInterface && !viewOnly) { + // this should be called before updateTransactions() + await (this as SparkInterface).refreshSparkData((0.3, 0.6)); + } - // await getAllTxsToWatch(); + if (this is NamecoinWallet) { + await updateUTXOs(); + _fireRefreshPercentChange(0.6); + await (this as NamecoinWallet).checkAutoRegisterNameNewOutputs(); + _fireRefreshPercentChange(0.70); + await updateTransactions(); + } else { + final fetchFuture = updateTransactions(); + _fireRefreshPercentChange(0.6); + final utxosRefreshFuture = updateUTXOs(); + _fireRefreshPercentChange(0.65); + await utxosRefreshFuture; + _fireRefreshPercentChange(0.70); + await fetchFuture; + } - _fireRefreshPercentChange(0.90); + // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. + if (!viewOnly && this is PaynymInterface && codesToCheck.isNotEmpty) { + await (this as PaynymInterface).checkForNotificationTransactionsTo( + codesToCheck, + ); + // check utxos again for notification outputs + await updateUTXOs(); + } + _fireRefreshPercentChange(0.80); - await updateBalance(); + // await getAllTxsToWatch(); - _fireRefreshPercentChange(1.0); + _fireRefreshPercentChange(0.90); - completer.complete(); - } catch (error, strace) { - completer.completeError(error, strace); - } finally { - refreshMutex.release(); - if (!completer.isCompleted) { - completer.completeError( - "finally block hit before completer completed", - StackTrace.current, - ); - } + await updateBalance(); - Logging.instance.i( - "Refresh for " - "$walletId::${info.name}: ${DateTime.now().difference(start)}", - ); - } + _fireRefreshPercentChange(1.0); } Future exit() async { From ab2823e18a61e8a3462092def26cbb2e81ec34b9 Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Mon, 20 Apr 2026 16:02:04 +0800 Subject: [PATCH 2/7] fix: fire refresh progress updates after awaited work completes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: 0.6 → 0.65 → [await UTXOs] → 0.70 → [await txns] After: 0.6 → [await UTXOs] → 0.65 → [await txns] → 0.70 Previously the 0.65 and 0.70 updates fired immediately after kicking off updateUTXOs and updateTransactions rather than after awaiting them, making the progress bar appear stuck at those values while the real work was still running. The Firo sync "stuck at 65%" reports were the most visible manifestation. Namecoin branch is unaffected (it already awaits before firing). Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/wallets/wallet/wallet.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/wallets/wallet/wallet.dart b/lib/wallets/wallet/wallet.dart index daaf017d5..c46e5962d 100644 --- a/lib/wallets/wallet/wallet.dart +++ b/lib/wallets/wallet/wallet.dart @@ -729,10 +729,10 @@ abstract class Wallet { final fetchFuture = updateTransactions(); _fireRefreshPercentChange(0.6); final utxosRefreshFuture = updateUTXOs(); - _fireRefreshPercentChange(0.65); await utxosRefreshFuture; - _fireRefreshPercentChange(0.70); + _fireRefreshPercentChange(0.65); await fetchFuture; + _fireRefreshPercentChange(0.70); } // TODO: [prio=low] handle this differently. Extra modification of this file for coin specific functionality should be avoided. From b91464a8ff4e8232382dfc5713c23e04a516ed37 Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Mon, 20 Apr 2026 16:03:52 +0800 Subject: [PATCH 3/7] fix: add idle watchdog to _refresh() so hung syncs release the mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, if any awaited sub-operation inside _refresh() hung indefinitely (unresponsive server, OS-suspended socket, wedged native-lib callback, etc.), refreshMutex was never released: the catch/finally blocks only run when the future completes or throws, and a permanently pending future does neither. Every subsequent periodic sync then bailed out at the isLocked check, leaving the wallet unable to sync until the app was force-closed. The Firo "stuck at ~65%" reports were the most visible manifestation. _refresh() now races _doRefreshWork(viewOnly) against a Timer.periodic watchdog. The watchdog trips if no refresh activity has been observed for 5 minutes, throwing TimeoutException through the existing catch/finally so the mutex is released and the next periodic sync can proceed. "Activity" is fed from two sources: - _fireRefreshPercentChange calls (coarse phase checkpoints plus Spark per-sector progress during anon-set downloads), via _lastRefreshProgress. - Successful electrum RPCs, via a new onRequestComplete hook on ElectrumXClient. This covers the case where updateTransactions() on a big wallet does hundreds of sequential getTransaction RPCs with no _fireRefreshPercentChange calls between 0.65 and 0.70 — on a slow server that would otherwise look idle for >5 min and trip the watchdog falsely. The hook is wired only when the wallet implements ElectrumXInterface, cleared in _refresh()'s inner finally, and covers all current and future electrum-routed calls automatically (request + batchRequest). Per-call hang detection remains the responsibility of the underlying adapters (e.g. electrum's connectionTimeout / aliveTimerDuration at 60s each). The watchdog only catches what slips through those layers. Note: the watchdog does not cancel in-flight work; it only unblocks the outer future so the mutex can be released. The orphaned work eventually resolves or errors on its own. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/electrumx_rpc/electrumx_client.dart | 8 ++++ lib/wallets/wallet/wallet.dart | 60 ++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/lib/electrumx_rpc/electrumx_client.dart b/lib/electrumx_rpc/electrumx_client.dart index b7c52b7d5..5906e1e21 100644 --- a/lib/electrumx_rpc/electrumx_client.dart +++ b/lib/electrumx_rpc/electrumx_client.dart @@ -117,6 +117,12 @@ class ElectrumXClient { final Mutex _torConnectingLock = Mutex(); bool _requireMutex = false; + /// Optional hook fired after each successful RPC completes. Used by + /// higher layers (e.g. the wallet refresh idle watchdog) to detect + /// liveness during long sequential RPC loops without having to + /// instrument every call site. + void Function()? onRequestComplete; + ElectrumXClient({ required String host, required int port, @@ -368,6 +374,7 @@ class ElectrumXClient { } currentFailoverIndex = -1; + onRequestComplete?.call(); // If the command is a ping, a good return should always be null. if (command.contains("ping")) { @@ -474,6 +481,7 @@ class ElectrumXClient { // } currentFailoverIndex = -1; + onRequestComplete?.call(); return response; } on WifiOnlyException { rethrow; diff --git a/lib/wallets/wallet/wallet.dart b/lib/wallets/wallet/wallet.dart index c46e5962d..2a74d1730 100644 --- a/lib/wallets/wallet/wallet.dart +++ b/lib/wallets/wallet/wallet.dart @@ -108,6 +108,10 @@ abstract class Wallet { Timer? _periodicRefreshTimer; Timer? _networkAliveTimer; + /// Timestamp of the last _fireRefreshPercentChange during an active + /// refresh. Consumed by the idle watchdog in _refresh() to detect hangs. + DateTime? _lastRefreshProgress; + bool _shouldAutoSync = false; bool _isConnected = false; @@ -603,6 +607,7 @@ abstract class Wallet { } void _fireRefreshPercentChange(double percent) { + _lastRefreshProgress = DateTime.now(); if (this is ElectrumXInterface) { (this as ElectrumXInterface?)?.refreshingPercent = percent; } @@ -641,7 +646,60 @@ abstract class Wallet { ); } - await _doRefreshWork(viewOnly); + // Idle watchdog: trips when no refresh activity has been observed + // for idleThreshold, signalling that the refresh is wedged. + // Slow-but-active syncs keep the watchdog fed and aren't killed: + // - _fireRefreshPercentChange ticks (e.g. Spark per-sector progress) + // - successful electrum RPCs (via ElectrumXClient.onRequestComplete) + // Per-call hang detection is still the responsibility of the + // underlying adapters (e.g. electrum's connectionTimeout). This only + // catches what slips through those layers and would otherwise hold + // refreshMutex locked until the app is force-closed. + const idleThreshold = Duration(minutes: 5); + _lastRefreshProgress = DateTime.now(); + + // Feed the watchdog from successful electrum RPCs, so long sequential + // fetches (e.g. updateTransactions on a wallet with a large history) + // are classified as active rather than idle. + if (this is ElectrumXInterface) { + (this as ElectrumXInterface).electrumXClient.onRequestComplete = () { + _lastRefreshProgress = DateTime.now(); + }; + } + + final watchdogCompleter = Completer(); + final watchdog = Timer.periodic(const Duration(seconds: 30), (timer) { + if (watchdogCompleter.isCompleted) { + timer.cancel(); + return; + } + final last = _lastRefreshProgress; + if (last == null) return; + if (DateTime.now().difference(last) >= idleThreshold) { + timer.cancel(); + watchdogCompleter.completeError( + TimeoutException( + 'Wallet refresh for $walletId idle for ' + '${idleThreshold.inMinutes} min', + idleThreshold, + ), + ); + } + }); + + try { + await Future.any([ + _doRefreshWork(viewOnly), + watchdogCompleter.future, + ]); + } finally { + watchdog.cancel(); + if (this is ElectrumXInterface) { + (this as ElectrumXInterface).electrumXClient.onRequestComplete = + null; + } + _lastRefreshProgress = null; + } completer.complete(); } catch (error, strace) { From 44339df011ddae2d299ea8799395ca891c56b026 Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Wed, 22 Apr 2026 22:48:19 +0800 Subject: [PATCH 4/7] fix: isolate onRequestComplete hook from RPC error handling Wrap the hook call in _fireOnRequestComplete() which swallows (and logs) exceptions. Previously a throw from the hook would land in the surrounding catch block and be misattributed as an RPC failure, incrementing currentFailoverIndex and triggering a failover retry against the next server despite the RPC having already succeeded. --- lib/electrumx_rpc/electrumx_client.dart | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/electrumx_rpc/electrumx_client.dart b/lib/electrumx_rpc/electrumx_client.dart index 5906e1e21..f405962c3 100644 --- a/lib/electrumx_rpc/electrumx_client.dart +++ b/lib/electrumx_rpc/electrumx_client.dart @@ -121,8 +121,24 @@ class ElectrumXClient { /// higher layers (e.g. the wallet refresh idle watchdog) to detect /// liveness during long sequential RPC loops without having to /// instrument every call site. + /// + /// Single-subscriber. Invoked via [_fireOnRequestComplete] which + /// swallows exceptions so a faulty hook cannot be misattributed as + /// an RPC failure by the surrounding catch block. void Function()? onRequestComplete; + void _fireOnRequestComplete() { + try { + onRequestComplete?.call(); + } catch (e, s) { + Logging.instance.w( + "onRequestComplete hook threw", + error: e, + stackTrace: s, + ); + } + } + ElectrumXClient({ required String host, required int port, @@ -374,7 +390,7 @@ class ElectrumXClient { } currentFailoverIndex = -1; - onRequestComplete?.call(); + _fireOnRequestComplete(); // If the command is a ping, a good return should always be null. if (command.contains("ping")) { @@ -481,7 +497,7 @@ class ElectrumXClient { // } currentFailoverIndex = -1; - onRequestComplete?.call(); + _fireOnRequestComplete(); return response; } on WifiOnlyException { rethrow; From 8c6e80983fe3aebb1a47570948df6cba549192cf Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Wed, 22 Apr 2026 22:48:44 +0800 Subject: [PATCH 5/7] fix: suppress uncaught errors from detached refresh work after watchdog trip When the watchdog wins the Future.any race, _doRefreshWork is abandoned but keeps running (Dart has no cooperative cancellation). If it later throws, the error lands on a detached future and surfaces as an uncaught async error. Attach a no-op catchError to the work future in the finally block so late failures are silently dropped. --- lib/wallets/wallet/wallet.dart | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/wallets/wallet/wallet.dart b/lib/wallets/wallet/wallet.dart index 2a74d1730..be5e40ace 100644 --- a/lib/wallets/wallet/wallet.dart +++ b/lib/wallets/wallet/wallet.dart @@ -687,11 +687,9 @@ abstract class Wallet { } }); + final work = _doRefreshWork(viewOnly); try { - await Future.any([ - _doRefreshWork(viewOnly), - watchdogCompleter.future, - ]); + await Future.any([work, watchdogCompleter.future]); } finally { watchdog.cancel(); if (this is ElectrumXInterface) { @@ -699,6 +697,12 @@ abstract class Wallet { null; } _lastRefreshProgress = null; + // If the watchdog won the race, `work` is detached and still + // running; an eventual throw would surface as an uncaught async + // error. Attach a no-op error handler to suppress it. If `work` + // completed first, this future is already resolved and the + // handler is a no-op. + unawaited(work.catchError((_) {})); } completer.complete(); From d06494713499974aeb9f86dfe0af819bbc82540d Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Wed, 22 Apr 2026 22:49:34 +0800 Subject: [PATCH 6/7] refactor: promote refresh watchdog intervals to named constants Pull the 5-minute idle threshold and 30-second watchdog tick out of the function body and onto the class as _refreshIdleThreshold and _refreshWatchdogTick. Makes both values greppable and trivially tunable. --- lib/wallets/wallet/wallet.dart | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/wallets/wallet/wallet.dart b/lib/wallets/wallet/wallet.dart index be5e40ace..762b9492b 100644 --- a/lib/wallets/wallet/wallet.dart +++ b/lib/wallets/wallet/wallet.dart @@ -105,6 +105,13 @@ abstract class Wallet { // ===== private properties =========================================== + /// Maximum time with no refresh activity before the idle watchdog + /// trips and unblocks _refresh() so refreshMutex can be released. + static const _refreshIdleThreshold = Duration(minutes: 5); + + /// How often the idle watchdog checks _lastRefreshProgress. + static const _refreshWatchdogTick = Duration(seconds: 30); + Timer? _periodicRefreshTimer; Timer? _networkAliveTimer; @@ -647,7 +654,7 @@ abstract class Wallet { } // Idle watchdog: trips when no refresh activity has been observed - // for idleThreshold, signalling that the refresh is wedged. + // for _refreshIdleThreshold, signalling that the refresh is wedged. // Slow-but-active syncs keep the watchdog fed and aren't killed: // - _fireRefreshPercentChange ticks (e.g. Spark per-sector progress) // - successful electrum RPCs (via ElectrumXClient.onRequestComplete) @@ -655,7 +662,6 @@ abstract class Wallet { // underlying adapters (e.g. electrum's connectionTimeout). This only // catches what slips through those layers and would otherwise hold // refreshMutex locked until the app is force-closed. - const idleThreshold = Duration(minutes: 5); _lastRefreshProgress = DateTime.now(); // Feed the watchdog from successful electrum RPCs, so long sequential @@ -668,20 +674,20 @@ abstract class Wallet { } final watchdogCompleter = Completer(); - final watchdog = Timer.periodic(const Duration(seconds: 30), (timer) { + final watchdog = Timer.periodic(_refreshWatchdogTick, (timer) { if (watchdogCompleter.isCompleted) { timer.cancel(); return; } final last = _lastRefreshProgress; if (last == null) return; - if (DateTime.now().difference(last) >= idleThreshold) { + if (DateTime.now().difference(last) >= _refreshIdleThreshold) { timer.cancel(); watchdogCompleter.completeError( TimeoutException( 'Wallet refresh for $walletId idle for ' - '${idleThreshold.inMinutes} min', - idleThreshold, + '${_refreshIdleThreshold.inMinutes} min', + _refreshIdleThreshold, ), ); } From e7ed2ae5d8cc1d9f6dbf4186b910eb910d979f81 Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Wed, 22 Apr 2026 22:56:45 +0800 Subject: [PATCH 7/7] =?UTF-8?q?docs:=20correct=20watchdog=20comment=20?= =?UTF-8?q?=E2=80=94=20Spark=20is=20covered=20by=20electrum=20hook,=20not?= =?UTF-8?q?=20percent=20ticks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spark_interface.dart's _triggerEventHelper fires progress events directly (refreshingPercent + GlobalEventBus) and does not call _fireRefreshPercentChange, so it does not update _lastRefreshProgress. Spark anon-set downloads are covered by the electrumXClient.onRequestComplete hook because they use electrumXClient internally. Correct the comment so future readers understand which path actually covers Spark. --- lib/wallets/wallet/wallet.dart | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/wallets/wallet/wallet.dart b/lib/wallets/wallet/wallet.dart index 762b9492b..7f5cf9c5a 100644 --- a/lib/wallets/wallet/wallet.dart +++ b/lib/wallets/wallet/wallet.dart @@ -656,8 +656,11 @@ abstract class Wallet { // Idle watchdog: trips when no refresh activity has been observed // for _refreshIdleThreshold, signalling that the refresh is wedged. // Slow-but-active syncs keep the watchdog fed and aren't killed: - // - _fireRefreshPercentChange ticks (e.g. Spark per-sector progress) + // - _fireRefreshPercentChange ticks (coarse phase checkpoints) // - successful electrum RPCs (via ElectrumXClient.onRequestComplete) + // — this covers Spark anon-set downloads and long updateTransactions + // loops, which use electrumXClient directly and do not call + // _fireRefreshPercentChange between phases. // Per-call hang detection is still the responsibility of the // underlying adapters (e.g. electrum's connectionTimeout). This only // catches what slips through those layers and would otherwise hold