offline 2fa wallet creation fails on macOS

[SomberNight]

On macOS, when using the release binary (4.5.4), and double-clicking the .app to start Electrum, offline 2fa wallet creation fails.
To trigger the bug, it is necessary to start Electrum by double-clicking the .app using the desktop environment, instead of using the terminal, as this results in os.getcwd() == "/".

In the second stage of the offline 2fa wallet creation (so during the online part), see snippet:

electrum/electrum/gui/qt/wizard/wallet.py

Lines 182 to 186 in 32d5e17

	wallet_file = wizard_data['wallet_name']
	storage = WalletStorage(wallet_file)
	if not storage.is_encrypted_with_user_pw() and not storage.is_encrypted_with_hw_device():
	return True

• wizard_data['wallet_name'] is the file basename, instead of a full path
• WalletStorage(wallet_file) will start creating a "new file"
• WalletStorage.__init__ will call standardize_path, which calls os.path.abspath(), which uses os.getcwd() to convert the filename to an abs path
  • this is a logic bug, and depending on cwd, either a hard error, or silent soft error will follow:
    • on macOS with the .app double-click start, cwd is /, and test_read_write_permissions will error:
      

      electrum/electrum/storage.py

      Line 71 in 32d5e17

      test_read_write_permissions(self.path)

      • btw on Windows, double-clicking the .exe sets cwd to the location of the .exe
    • on other systems, or if cwd is something nicer, WalletStorage.__init__ will succeed as if we were creating a new file, and QENewWalletWizard.is_finalized() will exit successfully on line 186:
      

      electrum/electrum/gui/qt/wizard/wallet.py

      Lines 185 to 186 in 32d5e17

      	if not storage.is_encrypted_with_user_pw() and not storage.is_encrypted_with_hw_device():
      	return True

Traceback (most recent call last):
  File "electrum/gui/qt/__init__.py", line 441, in _start_wizard_to_select_or_create_wallet
  File "electrum/daemon.py", line 487, in func_wrapper
  File "electrum/daemon.py", line 497, in load_wallet
  File "electrum/util.py", line 482, in do_profile
  File "electrum/daemon.py", line 522, in _load_wallet
electrum.wallet_db.WalletUnfinished

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "electrum/util.py", line 2004, in test_read_write_permissions
OSError: [Errno 30] Read-only file system: '/wallet_4.tmptest.905'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "electrum/storage.py", line 71, in __init__
  File "electrum/util.py", line 2010, in test_read_write_permissions
OSError: [Errno 30] Read-only file system: '/wallet_4.tmptest.905'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "electrum/gui/qt/wizard/wizard.py", line 212, in on_next_button_clicked
  File "electrum/gui/qt/wizard/wallet.py", line 184, in is_finalized
  File "electrum/storage.py", line 73, in __init__
electrum.storage.StorageReadWriteError: [Errno 30] Read-only file system: '/wallet_4.tmptest.905'
 
Additional information
Electrum version: 4.5.4 
Python version: 3.10.11 (v3.10.11:7d4cc5aa85, Apr 4 2023, 19:05:19) [Clang 13.0.0 (clang-1300.0.29.30)] 
Operating system: macOS-12.5-x86_64-i386-64bit 
Wallet type: standard 
Locale: ? 

related: #8815 (comment)

---

In either case, the problem is that WalletStorage(wallet_file) intends to create a storage for an existing file, but instead, due to confusion with the file paths, it ends up creating a new storage. The following patch illustrates the problem -- the assert will trigger:

diff --git a/electrum/gui/qt/wizard/wallet.py b/electrum/gui/qt/wizard/wallet.py
index 8b5b6e084b..4075765a93 100644
--- a/electrum/gui/qt/wizard/wallet.py
+++ b/electrum/gui/qt/wizard/wallet.py
@@ -182,6 +182,7 @@ class QENewWalletWizard(NewWalletWizard, QEAbstractWizard, MessageBoxMixin):
         wallet_file = wizard_data['wallet_name']
 
         storage = WalletStorage(wallet_file)
+        assert storage.file_exists(), f"file {wallet_file!r} does not exist"
         if not storage.is_encrypted_with_user_pw() and not storage.is_encrypted_with_hw_device():
             return True

Remarks:

1. Perhaps we should have both wizard_data['wallet_name'] and wizard_data['wallet_path']. Searching for existing usages of wizard_data['wallet_name'] shows that in some cases it contains a full path, and in other cases it contains just the basename.
2. Looks like WalletStorage.__init__ is not a good API: it mixes creating new files and opening existing files. Perhaps we should be explicit and have two separate APIs. That would reveal and prevent similar bugs.

[SomberNight]

somewhat related: 41bb849