Monero: let the user close the rescan dialog and never leak the refresh lock

lib/pages/settings_views/wallet_settings_view/wallet_network_settings_view/sub_widgets/rescanning_dialog.dart

@@ -9,6 +9,8 @@
  */
 
 import 'package:flutter/material.dart';
+import '../../../../../themes/stack_colors.dart';
+import '../../../../../utilities/text_styles.dart';
 import '../../../../../utilities/util.dart';
 import '../../../../../widgets/animated_widgets/rotating_arrows.dart';
 import '../../../../../widgets/conditional_parent.dart';
@@ -18,9 +20,16 @@ import '../../../../../widgets/stack_dialog.dart';
 class RescanningDialog extends StatefulWidget {
   const RescanningDialog({
     super.key,
+    this.rescansInBackground = false,
+    this.onClose,
     // required this.onCancel,
   });
 
+  /// If true the rescan carries on in the background (see the sync status)
+  /// so the user is allowed to close this dialog and leave.
+  final bool rescansInBackground;
+  final VoidCallback? onClose;
+
   // final VoidCallback onCancel;
 
   @override
@@ -65,22 +74,41 @@ class _RescanningDialogState extends State<RescanningDialog>
   Widget build(BuildContext context) {
     return WillPopScope(
       onWillPop: () async {
-        return false;
+        if (widget.rescansInBackground) widget.onClose?.call();
+        return widget.rescansInBackground;
       },
       child: ConditionalParent(
         condition: isDesktop,
         builder: (child) => DesktopDialog(
-          maxHeight: 150,
+          maxHeight: widget.rescansInBackground ? 230 : 150,
           maxWidth: 500,
           child: child,
         ),
-        child: const StackDialog(
+        child: StackDialog(
           title: "Rescanning blockchain",
-          message: "This may take a while. Please do not exit this screen.",
-          icon: RotatingArrows(
+          message: widget.rescansInBackground
+              ? "This may take a while. You can close this dialog, the rescan "
+                    "will continue in the background."
+              : "This may take a while. Please do not exit this screen.",
+          icon: const RotatingArrows(
             width: 24,
             height: 24,
           ),
+          rightButton: widget.rescansInBackground
+              ? TextButton(
+                  style: Theme.of(context)
+                      .extension<StackColors>()!
+                      .getSecondaryEnabledButtonStyle(context),
+                  child: Text(
+                    "Close",
+                    style: STextStyles.itemSubtitle12(context),
+                  ),
+                  onPressed: () {
+                    widget.onClose?.call();
+                    Navigator.of(context).pop();
+                  },
+                )
+              : null,
           // rightButton: TextButton(
           //   style: Theme.of(context).textButtonTheme.style?.copyWith(
           //     backgroundColor: MaterialStateProperty.all<Color>(

lib/pages/settings_views/wallet_settings_view/wallet_network_settings_view/wallet_network_settings_view.dart

@@ -136,23 +136,33 @@ class _WalletNetworkSettingsViewState
 
     try {
       if (mounted) {
+        final wallet = ref.read(pWallets).getWallet(widget.walletId);
+        final rescansInBackground = wallet is CryptonoteWallet;
+
+        // Cryptonote wallets keep rescanning after recover is called, so the
+        // user may close the dialog before recover returns if a refresh is in
+        // progress.
+        bool rescanningDialogOpen = true;
         unawaited(
           showDialog<dynamic>(
             context: context,
             useSafeArea: false,
             barrierDismissible: false,
-            builder: (context) => const RescanningDialog(),
-          ),
+            builder: (context) => RescanningDialog(
+              rescansInBackground: rescansInBackground,
+              onClose: () => rescanningDialogOpen = false,
+            ),
+          ).then((_) => rescanningDialogOpen = false),
         );
 
         try {
-          final wallet = ref.read(pWallets).getWallet(widget.walletId);
-
           await wallet.recover(isRescan: true);
 
           if (mounted) {
             // pop rescanning dialog
-            Navigator.of(context, rootNavigator: isDesktop).pop();
+            if (rescanningDialogOpen) {
+              Navigator.of(context, rootNavigator: isDesktop).pop();
+            }
 
             final String message;
             if (wallet is CryptonoteWallet || wallet is EpiccashWallet) {
@@ -193,7 +203,9 @@ class _WalletNetworkSettingsViewState
 
           if (mounted) {
             // pop rescanning dialog
-            Navigator.of(context, rootNavigator: isDesktop).pop();
+            if (rescanningDialogOpen) {
+              Navigator.of(context, rootNavigator: isDesktop).pop();
+            }
 
             // show error
             await showDialog<dynamic>(

lib/wallets/wallet/intermediate/lib_monero_wallet.dart

@@ -392,8 +392,10 @@ abstract class LibMoneroWallet<T extends CryptonoteCurrency>
         // clear blockchain info
         await mainDB.deleteWalletBlockchainData(walletId);
 
-        highestPercentCached = 0;
         await csMonero.rescanBlockchain(wallet!);
+        // Reset after the worker has accepted the rescan so a progress reading sent
+        // before it cannot be ordered after the reset and pin the guard near 100%.
+        highestPercentCached = 0;
         await csMonero.startSyncing(wallet!);
         unawaited(save());
       });
@@ -881,10 +883,6 @@ abstract class LibMoneroWallet<T extends CryptonoteCurrency>
     final _syncStatus = syncStatus;
 
     if (_syncStatus != null) {
-      if (_syncStatus.progress() == 1 && refreshMutex.isLocked) {
-        refreshMutex.release();
-      }
-
       WalletSyncStatus? status;
       xmrAndWowSyncSpecificFunctionThatShouldBeGottenRidOfInTheFuture(true);
 
@@ -1124,41 +1122,45 @@ abstract class LibMoneroWallet<T extends CryptonoteCurrency>
       "$runtimeType refresh() refreshMutex.acquire() acquired!",
     );
 
-    Logging.instance.t("$runtimeType refresh() final node = getCurrentNode();");
-    final node = getCurrentNode();
-
-    Logging.instance.i(
-      "$runtimeType refresh() await _torNodeMismatchGuard(node)",
-    );
-    if (await _torNodeMismatchGuard(node)) {
-      throw Exception("TOR – clearnet mismatch");
-    }
+    try {
+      Logging.instance.t(
+        "$runtimeType refresh() final node = getCurrentNode();",
+      );
+      final node = getCurrentNode();
 
-    Logging.instance.t(
-      "$runtimeType refresh() it csMonero.startSyncing(wallet!);",
-    );
-    await csMonero.startSyncing(wallet!);
-    Logging.instance.t(
-      "$runtimeType refresh() _setSyncStatus(lib_monero_compat.StartingSyncStatus());",
-    );
-    _setSyncStatus(lib_monero_compat.StartingSyncStatus());
+      Logging.instance.i(
+        "$runtimeType refresh() await _torNodeMismatchGuard(node)",
+      );
+      if (await _torNodeMismatchGuard(node)) {
+        throw Exception("TOR – clearnet mismatch");
+      }
 
-    Logging.instance.t("$runtimeType refresh() await updateTransactions();");
-    await updateTransactions();
-    Logging.instance.t("$runtimeType refresh() await updateBalance();");
-    await updateBalance();
+      Logging.instance.t(
+        "$runtimeType refresh() it csMonero.startSyncing(wallet!);",
+      );
+      await csMonero.startSyncing(wallet!);
+      Logging.instance.t(
+        "$runtimeType refresh() _setSyncStatus(lib_monero_compat.StartingSyncStatus());",
+      );
+      _setSyncStatus(lib_monero_compat.StartingSyncStatus());
 
-    Logging.instance.t(
-      "$runtimeType refresh() await checkReceivingAddressForTransactions();",
-    );
-    if (info.otherData[WalletInfoKeys.reuseAddress] != true) {
-      await checkReceivingAddressForTransactions();
-    }
+      Logging.instance.t("$runtimeType refresh() await updateTransactions();");
+      await updateTransactions();
+      Logging.instance.t("$runtimeType refresh() await updateBalance();");
+      await updateBalance();
 
-    Logging.instance.t(
-      "$runtimeType refresh() refreshMutex.isLocked=${refreshMutex.isLocked} pre release.",
-    );
-    if (refreshMutex.isLocked) {
+      Logging.instance.t(
+        "$runtimeType refresh() await checkReceivingAddressForTransactions();",
+      );
+      if (info.otherData[WalletInfoKeys.reuseAddress] != true) {
+        await checkReceivingAddressForTransactions();
+      }
+    } finally {
+      // always release, otherwise a failed refresh leaves the lock held
+      // forever and blocks rescan (recover) and every later refresh
+      Logging.instance.t(
+        "$runtimeType refresh() refreshMutex.isLocked=${refreshMutex.isLocked} pre release.",
+      );
       refreshMutex.release();
       Logging.instance.t(
         "$runtimeType refresh() refreshMutex.isLocked manually released.",

lib/wallets/wallet/intermediate/lib_salvium_wallet.dart

@@ -862,10 +862,6 @@ abstract class LibSalviumWallet<T extends CryptonoteCurrency>
     final _syncStatus = syncStatus;
 
     if (_syncStatus != null) {
-      if (_syncStatus.progress() == 1 && refreshMutex.isLocked) {
-        refreshMutex.release();
-      }
-
       WalletSyncStatus? status;
       xmrAndWowSyncSpecificFunctionThatShouldBeGottenRidOfInTheFuture(true);
 
@@ -1101,17 +1097,17 @@ abstract class LibSalviumWallet<T extends CryptonoteCurrency>
     // Slight possibility of race but should be irrelevant
     await refreshMutex.acquire();
 
-    csSalvium.startSyncing(wallet!);
-    _setSyncStatus(StartingSyncStatus());
-
-    await updateTransactions();
-    await updateBalance();
+    try {
+      csSalvium.startSyncing(wallet!);
+      _setSyncStatus(StartingSyncStatus());
 
-    if (info.otherData[WalletInfoKeys.reuseAddress] != true) {
-      await checkReceivingAddressForTransactions();
-    }
+      await updateTransactions();
+      await updateBalance();
 
-    if (refreshMutex.isLocked) {
+      if (info.otherData[WalletInfoKeys.reuseAddress] != true) {
+        await checkReceivingAddressForTransactions();
+      }
+    } finally {
       refreshMutex.release();
     }
 

lib/wallets/wallet/intermediate/lib_wownero_wallet.dart

@@ -898,10 +898,6 @@ abstract class LibWowneroWallet<T extends CryptonoteCurrency>
     final _syncStatus = syncStatus;
 
     if (_syncStatus != null) {
-      if (_syncStatus.progress() == 1 && refreshMutex.isLocked) {
-        refreshMutex.release();
-      }
-
       WalletSyncStatus? status;
       xmrAndWowSyncSpecificFunctionThatShouldBeGottenRidOfInTheFuture(true);
 
@@ -1138,17 +1134,17 @@ abstract class LibWowneroWallet<T extends CryptonoteCurrency>
     // Slight possibility of race but should be irrelevant
     await refreshMutex.acquire();
 
-    csWownero.startSyncing(wallet!);
-    _setSyncStatus(lib_monero_compat.StartingSyncStatus());
-
-    await updateTransactions();
-    await updateBalance();
+    try {
+      csWownero.startSyncing(wallet!);
+      _setSyncStatus(lib_monero_compat.StartingSyncStatus());
 
-    if (info.otherData[WalletInfoKeys.reuseAddress] != true) {
-      await checkReceivingAddressForTransactions();
-    }
+      await updateTransactions();
+      await updateBalance();
 
-    if (refreshMutex.isLocked) {
+      if (info.otherData[WalletInfoKeys.reuseAddress] != true) {
+        await checkReceivingAddressForTransactions();
+      }
+    } finally {
       refreshMutex.release();
     }
 

test/widget_tests/rescanning_dialog_test.dart

@@ -0,0 +1,49 @@
+import 'package:flutter/material.dart';
+import 'package:flutter_test/flutter_test.dart';
+import 'package:stackwallet/models/isar/stack_theme.dart';
+import 'package:stackwallet/pages/settings_views/wallet_settings_view/wallet_network_settings_view/sub_widgets/rescanning_dialog.dart';
+import 'package:stackwallet/themes/stack_colors.dart';
+
+import '../sample_data/theme_json.dart';
+
+void main() {
+  testWidgets("marks a background rescan closed before dismissing", (
+    tester,
+  ) async {
+    final theme = StackTheme.fromJson(json: lightThemeJsonMap);
+    var closed = false;
+
+    await tester.pumpWidget(
+      MaterialApp(
+        theme: ThemeData(extensions: [StackColors.fromStackColorTheme(theme)]),
+        home: Builder(
+          builder: (context) => TextButton(
+            onPressed: () => showDialog<void>(
+              context: context,
+              builder: (_) => RescanningDialog(
+                rescansInBackground: true,
+                onClose: () {
+                  expect(find.byType(RescanningDialog), findsOneWidget);
+                  closed = true;
+                },
+              ),
+            ),
+            child: const Text("Open"),
+          ),
+        ),
+      ),
+    );
+    await tester.tap(find.text("Open"));
+    await tester.pump();
+    await tester.pump(const Duration(milliseconds: 500));
+
+    tester
+        .widget<TextButton>(find.widgetWithText(TextButton, "Close"))
+        .onPressed!();
+    await tester.pump();
+    await tester.pump(const Duration(milliseconds: 500));
+
+    expect(closed, isTrue);
+    expect(find.byType(RescanningDialog), findsNothing);
+  });
+}