You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
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:
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: ?
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:
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.
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.
The text was updated successfully, but these errors were encountered:
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.
This is a bit of legacy unfortunately ported over from the old wizard. The most consistent solution IMO is to convert to a fully qualified path immediately and use that everywhere. The bare name is easy to extract in places where that is applicable.
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.
agreed. See also this comment w.r.t. wallet path inconsistencies: #8901 (comment)
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
wizard_data['wallet_name']
is the file basename, instead of a full pathWalletStorage(wallet_file)
will start creating a "new file"WalletStorage.__init__
will callstandardize_path
, which callsos.path.abspath()
, which usesos.getcwd()
to convert the filename to an abs pathcwd
, either a hard error, or silent soft error will follow:/
, andtest_read_write_permissions
will error:electrum/electrum/storage.py
Line 71 in 32d5e17
WalletStorage.__init__
will succeed as if we were creating a new file, andQENewWalletWizard.is_finalized()
will exit successfully on line 186:electrum/electrum/gui/qt/wizard/wallet.py
Lines 185 to 186 in 32d5e17
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:Remarks:
wizard_data['wallet_name']
andwizard_data['wallet_path']
. Searching for existing usages ofwizard_data['wallet_name']
shows that in some cases it contains a full path, and in other cases it contains just the basename.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.The text was updated successfully, but these errors were encountered: